Skip to content

fix(rest): keep a table-scoped token off the shared session - #3999

Closed
ghoshp83 wants to merge 1 commit into
apache:mainfrom
ghoshp83:rest-session-header-isolation
Closed

ghoshp83 wants to merge 1 commit into
apache:mainfrom
ghoshp83:rest-session-header-isolation

Conversation

@ghoshp83

Copy link
Copy Markdown

Addresses the first half of #3970.

Rationale for this change

RestCatalog.commit_table bound the session's own header mapping and then wrote
the table-scoped token into it:

headers = self._session.headers            # the live mapping, not a copy
if table_token := table.config.get(TOKEN):
    headers[AUTHORIZATION_HEADER] = f"{BEARER_PREFIX} {table_token}"

self._session is shared by every table the catalog serves, so the assignment
outlives the commit. The token stays on the session and is carried by later
requests, including requests for other tables.

headers is already passed explicitly to self._session.post(...), so building
it from a copy sends exactly the same request while leaving the session alone.

Are these changes tested?

Yes — test_commit_table_does_not_leak_table_token_onto_session in
tests/catalog/test_rest.py loads a table whose config carries a token, commits,
and asserts the session has no Authorization header afterwards. On main the
assertion fails, because the session is left holding
Bearer table_scoped_token.

tests/catalog/test_rest.py passes (166 passed; the one unrelated failure,
test_rest_catalog_with_google_credentials_path, is a missing optional google
dependency in my environment and fails identically on main).

Are there any user-facing changes?

No. The request the catalog sends is unchanged; only the lifetime of the header
changes.

Left out of this PR

Two related findings, kept separate so this stays one concern:

commit_table bound self._session.headers and wrote the table's token into
it, so the token outlived the commit and was sent with later requests for
other tables. Build the request headers from a copy instead; the request
itself is unchanged.
@ghoshp83

Copy link
Copy Markdown
Author

Closing this in favour of #3976, which was opened two days earlier and covers
both halves of #3970 — the commit_table session mutation that this PR fixed,
and the cached S3FileSystem signer that it did not.

I missed #3976 when I checked for existing work: it links the issue from its body
rather than its title, and it never commented on #3970, so my scan of issue
comments and PR titles came up empty. That is my error, not a problem with the
PR.

No objection to #3976's approach — applying the table token through a
request-local auth callable is the more complete fix, since Session.auth
overwrites a per-request Authorization header and the plain header assignment
never reached the wire. My note on #3970 about that behaviour still stands as
background for anyone reviewing it.

@ghoshp83 ghoshp83 closed this Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant