Skip to content

Fix cupsOAuthGetUserId cache key, lifetime, and status checks - #190

Closed
anabobillo3 wants to merge 1 commit into
OpenPrinting:masterfrom
anabobillo3:oauth-userid-cache
Closed

anabobillo3 wants to merge 1 commit into
OpenPrinting:masterfrom
anabobillo3:oauth-userid-cache

Conversation

@anabobillo3

@anabobillo3 anabobillo3 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

cupsOAuthGetUserId keeps the user ID for an access token in the OAuth store, keyed by the token's urn:token: URI. Refiled from GHSA-84cm-m3c6-56gp as a regular report, as asked there.

What it does now (master 5e60665):

  • Key: oauth_make_path handles a urn:token: URI the way it handles an IPPS URI: it copies the text from offset 7 (en: and the token) into a 1024-byte buffer, cuts it at the first / and hashes the rest. So the key covers only the token up to its first /, and at most its first 1020 characters; two access tokens that agree up to there share one cached user ID.
  • Lifetime: the user ID that cupsOAuthSaveTokens stores for an access token has no expiration time, and cupsOAuthGetUserId returns a cached value without checking one, so it is still returned after the access token's access_expires has passed. cupsOAuthClearTokens clears the user ID stored for the resource URI, not the one stored for the access token.
  • userinfo: when nothing is cached, the userinfo_endpoint response is used whenever its JSON has no error member, whatever the HTTP status, and without a sub claim. The result is then not cached, since cupsJWTExportString returns NULL for the unsigned JWT and oauth_save_value takes NULL as a delete. The connection to the endpoint is not closed.

What this changes:

  • oauth_make_path hashes the whole urn:token: URI.
  • oauth_save_value takes an optional expiration time, written on a second line after the value. cupsOAuthSaveTokens uses it for the access token (instead of formatting it in a 16k buffer) and for the user ID it stores for the access token, with the access token's expiration time, otherwise five minutes (_CUPS_OAUTH_USERID_TIMEOUT); cupsOAuthGetUserId uses it for the user ID from the userinfo endpoint, with five minutes. cupsOAuthGetUserId uses a cached user ID only before that time and only with a sub claim; otherwise it removes it and asks the userinfo endpoint again. A user ID cached by an earlier version has no expiration time, so it is fetched again once.
  • From the userinfo endpoint, only a 200 OK response with a sub claim is used. Its claims are cached as JSON (cupsJSONExportString) and made into a JWT again with cupsJWTNew when loaded. The connection is closed.
  • The cupsOAuthGetUserId documentation says how long the user ID is cached.

Testing: checked offline with a private store: cupsOAuthGetUserId returns the cached user ID for its own access token, but no longer for a different token with the same text before its /, for a 1100-byte token that differs only in its last character, or once the access token has expired (without the oauth.c changes it returns the cached ID in those three cases). make test results are unchanged. As asked in the review, the PR adds no separate test of the caching code.

Assisted-by: Claude Code:claude-opus-5-5

@michaelrsweet michaelrsweet self-assigned this Oct 22, 2026
@michaelrsweet michaelrsweet added the investigating Investigating the issue label Oct 22, 2026

@michaelrsweet michaelrsweet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs work, and I really don't want to add more (separate) tests of the caching code.

Will provide more feedback as time allows (dealing with a rather high volume of reports these days with no end in sight...)

@anabobillo3

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look. I've removed the separate cache test and the Makefile change, so the PR now only touches cups/oauth.c. Whenever you have time to say what else needs work, I'll update it.

@michaelrsweet michaelrsweet added this to the Stable milestone Oct 23, 2026

@michaelrsweet michaelrsweet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I want to extend oauth_save_value to accept a time_t argument that provides an (optional) value that is written on a second line - we do that in cupsOAuthSaveTokens already for the access token expiration time...

Otherwise I like your changes.

@anabobillo3

Copy link
Copy Markdown
Contributor Author

Done: oauth_save_value now takes a time_t argument, and when it is non-zero it is written on a second line after the value. cupsOAuthSaveTokens uses it for the access token (so the temp buffer is gone) and for the user ID it saves for the access token, and cupsOAuthGetUserId uses it for the user ID it gets from the userinfo endpoint. oauth_save_user_id is removed, and the PR still only touches cups/oauth.c.

- oauth_make_path: hash the whole access token for "urn:token:" URIs instead
  of the text before the first '/' (and at most 1023 bytes), so different
  tokens no longer share a cached user ID.
- oauth_save_value: add an optional expiration time that is written on a
  second line after the value; cupsOAuthSaveTokens now uses it for the access
  token instead of formatting the token in a 16k buffer.
- cupsOAuthGetUserId: save an expiration time with the cached user ID (the
  access token's expiration time from cupsOAuthSaveTokens, otherwise five
  minutes) and get it again from the userinfo endpoint once it expires; only
  use a 200 OK response containing a "sub" claim; cache the userinfo claims as
  JSON since cupsJWTExportString no longer exports an unsigned JWT; close the
  userinfo connection.
@michaelrsweet

Copy link
Copy Markdown
Member

OK, largely taken as-is, but the oauth_make_path needed to retain the ipps: handling (HTTPS/IPPS equivalency) and I made some editorial changes:

[master ca531c0] Add expiration time to user_id cache (Issue #190)

@michaelrsweet michaelrsweet added bug Something isn't working and removed investigating Investigating the issue labels Nov 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants