Add /ia/sync.json POST handler

[jimchamp]

Addresses #7539

Creates new /api/unlink.json endpoint for the purpose of syncing data between IA and OL. POST requests to this endpoint are expected to have the following query parameters:

• msg : A colon delimited string in the format {ocaid}|{expiry}
• digest : The hashed value of msg

All inputs required for the operation are expected to be sent in the msg string.

The POST handler will, in order:

1. Verify the digest and check the expiry
2. Validate the request inputs
3. Query for the OL edition that matches the ocaid given in the msg
4. Perform the operation on the selected edition

Outstanding tasks

• Create new shared key for IA <-> OL sync operations
• Move verify_hmac to core/auth.py
• Disassociate MARC records

Technical

Special Deployment Instructions

Ensure that new shared key entry is added to our production configurations before deploying.

Testing

Screenshot

Stakeholders

[mekarpeles]

I admit I don't recall perfectly where we left off;

Is there a way we can try a working example on testing.openlibrary.org where we manually make a API call w/ a curl and a ocaid that @seabelis recommends, where we disassociate the item from Open Library?

This is annoying to test because it will require us manually minting an hmac token to send with the request based on some shared secret (and we may want to add/stage this secret for now in testing olsystem)

[jimchamp]

I admit I don't recall perfectly where we left off[...]

The three outstanding tasks from this comment still remain:

• A key must be generated and added to each application's vault/configurations.
• The verify_hmac function should be moved to a more appropriate place, especially if we plan on reusing it. Maybe one of our utils.py file would be a good place?
• The update_marc method remains unimplemented.

I was able to test verify_hmac locally by generating a hash with the following php:

<?php

$msg = "hello there";
$secret_key = "placeholderkey";

$expiry_time = date('c', time() + 60); // 'c' = ISO 8601 format
$data = $msg . "|" . $expiry_time;
$hash = hash_hmac('md5', $data, $secret_key);

echo "Hash: " . $hash . "\n";
echo "Data: " . $data . "\n";

?>

Have verified that an ExiredTokenError is thrown if the current time is greater than the expiry.

[mekarpeles]

As reference, we have One Time Password logic in this PR that is similar (living in core/auth.py) that could a home + model for how we might want to centralize our HMAC logic
#11288

Rather than waiting for 11288 to land, maybe the takeaway is we could use core/auth.py and have some sort of hmac class/utility to centralize how we either generate or verify an hmac.

[mekarpeles]

We have a new key ia_sync_secret https://github.com/internetarchive/olsystem/pull/287 that can be used for HMAC

[jimchamp]

Including the op in the encrypted message isn't necessary. This handler's path will be updated to something like /ia/unlink, and will handle all disassociation requests. Later, a complementary /ia/link handler will be created for association requests.

[jimchamp]

msg is now expected to by in <ocaid>:<timestamp format.

[Copilot]

Pull request overview

This PR introduces a new endpoint for syncing data between Internet Archive (IA) and Open Library (OL), specifically for "unlinking" items by removing OCAIDs from editions. The implementation includes HMAC-based authentication with timestamp validation to secure the endpoint.

Key Changes

• Creates new HMACToken authentication class in openlibrary/core/auth.py with HMAC verification and expiry checking
• Adds /ia/unlink POST endpoint in openlibrary/plugins/admin/code.py to remove OCAIDs from editions
• Implements make_dark operation to delete ocaid and ia: source records from editions

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.

File	Description
openlibrary/core/auth.py	New file containing HMACToken class for HMAC digest verification with timestamp validation and timing-attack mitigation
openlibrary/plugins/admin/code.py	Adds unlink_ia_ol endpoint class with POST handler for authenticated OCAID removal, including edition lookup and dark-making logic

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new HMACToken class lacks test coverage. Tests should be added to verify:

1. Successful verification with valid digest and non-expired timestamp
2. Rejection of expired tokens
3. Rejection of invalid digests
4. Handling of missing secret keys
5. Constant-time execution for timing attack mitigation
6. Proper parsing of delimiter-separated messages
7. Handling of malformed timestamps

[Copilot]

[nitpick] The authentication error handling catches both ValueError and ExpiredTokenError but treats them identically with a 401 response. This makes it difficult for clients to distinguish between expired tokens (which could be retried with a new token) and other authentication failures. Consider handling ExpiredTokenError separately with a more specific error message.

Suggested change

	except (ValueError, ExpiredTokenError):
	raise web.HTTPError("401 Unauthorized")
	except ExpiredTokenError:
	raise web.HTTPError(
	"401 Unauthorized",
	data=json.dumps({"error": "Token expired"})
	)
	except ValueError:
	raise web.HTTPError(
	"401 Unauthorized",
	data=json.dumps({"error": "Invalid token"})
	)

[Copilot]

The delimiter default value is documented as "|" but the PR description states the message format should use colon (:) as the delimiter. This inconsistency between documentation and the intended API design could lead to integration issues. Ensure the delimiter matches the agreed-upon API specification.

[mekarpeles]

@jimchamp please talk with @judec + @hbromley about how we want the hmc to be

[Copilot]

The PR description states the message format should be colon-delimited ({operation}:{ocaid}:{expiry}), but the code uses pipe delimiter (|). This mismatch means the implementation doesn't follow the documented API design. Additionally, the code doesn't parse or use an operation field at all - it only parses ocaid and ts and hardcodes the make_dark operation.

Suggested change

	ocaid, ts = msg.split("|")
	if not ts or not ocaid:
	raise web.HTTPError(
	"400 Bad Request", data=json.dumps({"error": "Invalid inputs"})
	)
	try:
	operation, ocaid, ts = msg.split(":")
	except ValueError:
	raise web.HTTPError(
	"400 Bad Request", data=json.dumps({"error": "Invalid message format; expected '{operation}:{ocaid}:{expiry}'"})
	)
	if not operation or not ocaid or not ts:
	raise web.HTTPError(
	"400 Bad Request", data=json.dumps({"error": "Invalid inputs"})
	)
	if operation != "make_dark":
	raise web.HTTPError(
	"400 Bad Request", data=json.dumps({"error": f"Unsupported operation: {operation}"})
	)

[Copilot]

The validation if not ts or not ocaid happens after splitting but doesn't handle potential parsing errors. If msg doesn't contain a pipe delimiter, the split will succeed but won't produce two values, causing an unpacking error on line 269. Consider adding error handling for the split operation or validating the number of components.

[Copilot]

The timing-attack mitigation logic is incomplete. When the secret key is missing (line 54-55), mac remains an empty string, which will always fail the compare_digest check on line 63. However, the error is only raised after the comparison (line 64-65). This means an attacker could distinguish between a missing key and an invalid digest by timing the response, since compare_digest on empty strings is faster than on actual hashes. Consider generating a dummy MAC value even when the key is missing to maintain constant-time execution.

Suggested change

	mac = ''
	if key:
	mac = hmac.new(
	key.encode('utf-8'), msg.encode('utf-8'), hashlib.md5
	).hexdigest()
	# Always produce a MAC of the correct length to mitigate timing attacks
	digest_size = hashlib.md5().digest_size * 2 # hexdigest is twice the digest size
	if key:
	mac = hmac.new(
	key.encode('utf-8'), msg.encode('utf-8'), hashlib.md5
	).hexdigest()
	else:
	# Use a dummy value of the correct length (all zeros)
	mac = '0' * digest_size

[Copilot]

The endpoint path /ia/unlink and class name unlink_ia_ol don't match the PR description which mentions /ia/sync.json endpoint. This inconsistency could cause confusion. Consider renaming to match the intended API design described in the PR.

Suggested change

	class unlink_ia_ol(delegate.page):
	path = '/ia/unlink'
	class sync_ia_ol(delegate.page):
	path = '/ia/sync.json'

[Copilot]

MD5 is considered cryptographically weak and should not be used for HMAC in security-sensitive contexts. Consider using a stronger hash algorithm like SHA-256 (hashlib.sha256) instead. While HMAC-MD5 is not as critically vulnerable as plain MD5, using SHA-256 is the modern standard and provides better security guarantees.

Suggested change

	key.encode('utf-8'), msg.encode('utf-8'), hashlib.md5
	key.encode('utf-8'), msg.encode('utf-8'), hashlib.sha256

[mekarpeles]

Strong agree + blocker

https://github.com/internetarchive/openlibrary/pull/11288/files#diff-7ff81128d3b27b26d5cf60f2d19a44042debc4b8c9b3788903cd1fb584104ea4R38

Let's use digest = hmac.new(seed.encode('utf-8'), payload, hashlib.sha256).digest()

[Copilot]

The verify method returns the result of hmac.compare_digest() but this return value is never checked in the calling code (line 265 in code.py). If the digest comparison fails but no error is set, the method returns False, but the caller doesn't handle this case - it only catches exceptions. This means an invalid digest would pass through without raising an error. Consider raising a ValueError when result is False before returning.

Suggested change

	return result
	if not result:
	raise ValueError("Invalid digest")
	return True

[Copilot]

The fromisoformat method can raise a ValueError if expiry_str is not a valid ISO format timestamp. This exception is not caught, which could lead to unclear error messages for the caller. Consider wrapping this in a try-except block to raise a more specific error or ensure the timing-attack mitigation logic still executes.

Suggested change

	expiry = datetime.datetime.fromisoformat(expiry_str)
	err: Exception | None = None
	if current_time > expiry:
	expiry = None
	err: Exception | None = None
	try:
	expiry = datetime.datetime.fromisoformat(expiry_str)
	except ValueError:
	err = ValueError(f"Invalid ISO format timestamp: {expiry_str!r}")
	if expiry is not None and current_time > expiry:

[jimchamp]

If the source_records list is empty after the MARC record is unlinked, source_records is deleted from the edition object.

[mekarpeles]

Merged but please let's (a) move to sha2 as per PR notes and (b) communicate with Hank and Jude re: the hmac format, timestamps, and delimiter