Skip to content

[auth] SecretValue review follow-ups: source leak and env fallback - #105

Open
ROHIT-JR wants to merge 2 commits into
nitrocloudofficial:developfrom
ROHIT-JR:fix/secret-value-review
Open

ROHIT-JR wants to merge 2 commits into
nitrocloudofficial:developfrom
ROHIT-JR:fix/secret-value-review

Conversation

@ROHIT-JR

@ROHIT-JR ROHIT-JR commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #93, fixing the two bugs from manish-wekan's review that were still on develop. The third point (the capture_locals test on Python 3.12) was already fixed on develop.

Fixes

  1. Pickle could leak a secret passed as source=. __reduce__ wrote source into the pickle as is. Now SecretValue(value, source=...) raises ValueError if the source contains the secret, and pickles only carry one of the known labels (env, config, explicit); anything else is written as custom.
  2. A redacted secret fell back to an environment variable. A secret that went through pickle holds no value and is falsy, so token_introspection_client_secret or os.environ.get(...) quietly replaced it. OAuthService now treats any SecretValue as "given", so unwrapping it raises instead of using the environment value. The same truthiness check was fixed in the introspection Basic-auth branch and in build_registration_response.

Tests

Five new tests in tests/test_secret_value.py. Three of them fail on current develop and pass with this change (source rejected, custom source never pickled, redacted secret not replaced by the environment). The OAuth fallback to the environment when no secret is given still works. Full suite: 882 passed, 16 skipped locally.

@Prateep07 Prateep07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The source allowlist and redacted pickle behavior address the ordinary-string case. One blocker remains before merge.

Change needed

A str subclass can still leak data through its pickle state. isinstance(source, str) accepts subclasses, and the allowlist branch returns the original source object when its value matches a safe label. A subclass’s custom pickle state can therefore include the secret. Please return a canonical built-in label (such as "env") instead of self.source, and add a regression test that confirms a subclass’s attached secret is absent from the pickle.

…eak state

__reduce__ returned self.source when it matched a safe label, so a str
subclass could carry a secret in its own pickle state. Now it emits a
built-in label chosen with str.__eq__, which ignores subclass overrides.
@ROHIT-JR

ROHIT-JR commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

@Prateep07 thanks, good catch. Fixed in 000918b.

__reduce__ no longer returns self.source. It emits a built-in label (env, config or explicit), chosen by comparing with str.__eq__, which ignores a subclass's own __eq__ and pickle state. Anything else is still written as custom.

Two regression tests, both failing on the previous commit:

  1. A str subclass equal to a safe label, with the secret in its pickle state. The secret is absent from the pickle and the label loads back as the plain built-in.
  2. A subclass that fakes == "env" but whose text is the secret. It is written as custom.

Full suite passes (884). Could you take another look?

@Prateep07
Prateep07 self-requested a review November 2, 2026 15:37

@Prateep07 Prateep07 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The follow-up safely serializes canonical source labels, closing the str subclass pickle-state leak. Regression tests cover both subclass state and overridden equality, and the secret-value test suite passes (47 tests).

@Prateep07
Prateep07 dismissed their stale review November 2, 2026 15:39

LGTM. The follow-up safely serializes canonical source labels, closing the str subclass pickle-state leak. Regression tests cover both subclass state and overridden equality, and the secret-value test suite passes (47 tests).

This branch has not been deployed

No deployments
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.

2 participants