fix(security): harden JWT validation, GCS path handling, and CI workflows#40
Open
williaby wants to merge 5 commits into
Open
fix(security): harden JWT validation, GCS path handling, and CI workflows#40williaby wants to merge 5 commits into
williaby wants to merge 5 commits into
Conversation
JWT validator (cloudflare-auth): - Enforce an explicit allowlist of asymmetric algorithms (RS*/ES*/PS*) at construction and again against the unverified token header, rejecting "none" and HS* to block algorithm-confusion / signature-stripping. - Make signature/exp/nbf/iat verification non-optional, add a "require" set for exp/iat/iss/sub/aud, and add 30s leeway for clock skew. - Replace blanket "except Exception" with PyJWT-specific exception handlers (InvalidTokenError catch-all) and log get_unverified_claims use loudly so misuse is visible. GCS client (gcs-utilities): - _sanitize_gcs_path now rejects control characters, backslashes, and "."/".." as path segments (plus any ".." substring). - Re-sanitize joined paths in upload_directory so symlink-introduced traversal sequences cannot bypass the prefix check. - delete_directory rejects empty / whitespace-only prefixes to prevent accidental bucket-wide wipes. - Replace bare "except Exception" in _get_or_create_bucket with the specific GoogleCloudError / OSError / ValueError set. GitHub workflows: - Pin every ByronWilliamsCPA/.github reusable-workflow reference to a commit SHA instead of @main. - Set workflow-level "permissions: contents: read" and grant elevated permissions only on the jobs that need them. - Add step-security/harden-runner with egress-policy: audit to every job that runs inline steps and did not already have it. Dependency review: - Add SECURITY-NOTES.md flagging that every dependency uses an unbounded ">=" floor and naming pyjwt, cryptography, and google-cloud-storage as the three highest-risk dependencies with rationale and suggested upper-bound constraints. https://claude.ai/code/session_01LfuLSQf3rcbYG1QdgL92a5
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
PyJWT's ``jwt.decode(..., algorithms=[...])`` already enforces the algorithm allowlist by reading the token header internally and raising ``InvalidAlgorithmError`` on mismatch. Combined with the constructor-time validation of ``settings.jwt_algorithm`` against ``_ALLOWED_JWT_ALGORITHMS``, the explicit ``_reject_unsafe_header_algorithm`` helper added in the previous commit was redundant. Removing it also removes a ``jwt.get_unverified_header()`` call that SonarCloud and CodeQL flag as a JWT-without-verification pattern, even though it was used defensively before the verified decode. No change in observable behaviour: tokens with unsafe ``alg`` values are still rejected (now via ``InvalidAlgorithmError`` -> ``ValueError`` in the existing exception handler). https://claude.ai/code/session_01LfuLSQf3rcbYG1QdgL92a5
Adds the missing test module for ``CloudflareJWTValidator``. Covers:
* Constructor algorithm allowlist accepts every entry in
``_ALLOWED_JWT_ALGORITHMS`` and rejects ``none``, HS*, lowercase
variants, and unknown values.
* ``is_configured`` reflects domain + audience presence; missing
team domain warns and leaves ``jwks_client`` as ``None``.
* ``validate_token`` raises ``RuntimeError`` when no JWKS client is
configured rather than silently passing.
* ``_validate_required_claims`` enforces email/iss/aud/sub/iat/exp.
* ``get_unverified_claims`` always logs a warning, returns the
decoded claims for a parseable token, and returns ``{}`` for
garbage input.
* ``refresh_keys`` swaps in a new PyJWKClient.
* ``validate_token`` passes ``verify_signature``/``verify_exp``/
``verify_nbf``/``verify_iat``/``require`` and an explicit
``algorithms`` list to ``jwt.decode``, plus non-zero leeway.
* Each PyJWT-specific exception is mapped to ``ValueError`` with a
descriptive message.
Brings patch coverage on ``validators.py`` from 13% up so the
codecov/patch gate clears for this PR.
https://claude.ai/code/session_01LfuLSQf3rcbYG1QdgL92a5
There was a problem hiding this comment.
Pull request overview
Security hardening pass for JWT validation, GCS path handling, GitHub Actions supply-chain posture, and dependency risk documentation.
Changes:
- Tightens JWT algorithm and claim validation behavior.
- Adds stricter GCS path/local path validation and safer directory deletion defaults.
- Pins reusable workflows, adjusts selected permissions, adds harden-runner steps, and documents dependency version-bound risks.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
SECURITY-NOTES.md |
Adds dependency risk notes and hardening summary. |
packages/cloudflare-auth/src/cloudflare_auth/validators.py |
Hardens JWT validation and exception handling. |
packages/gcs-utilities/src/gcs_utilities/client.py |
Strengthens path sanitization and deletion safeguards. |
.github/workflows/ci.yml |
Adds harden-runner and narrows job permissions. |
.github/workflows/codecov.yml |
Pins reusable workflow and hardens inline failure job. |
.github/workflows/coverage.yml |
Pins Qlty coverage reusable workflow. |
.github/workflows/dependency-review.yml |
Moves PR permission to job scope and adds harden-runner. |
.github/workflows/docs.yml |
Pins docs workflow and scopes deploy permissions to job. |
.github/workflows/fips-compatibility.yml |
Pins FIPS reusable workflow. |
.github/workflows/mutation-testing.yml |
Pins mutation-testing reusable workflow. |
.github/workflows/publish-artifact-registry.yml |
Moves OIDC permission to publish job and adds harden-runner. |
.github/workflows/python-compatibility.yml |
Pins compatibility reusable workflow. |
.github/workflows/qlty.yml |
Pins Qlty reusable workflow. |
.github/workflows/release.yml |
Narrows workflow permissions and hardens release jobs. |
.github/workflows/reuse.yml |
Pins REUSE reusable workflow. |
.github/workflows/sbom.yml |
Pins SBOM reusable workflow. |
.github/workflows/scorecard.yml |
Moves scorecard permissions to job scope and pins workflow. |
.github/workflows/security-analysis.yml |
Pins security analysis reusable workflow. |
.github/workflows/slsa-provenance.yml |
Pins SLSA reusable workflow. |
Comments suppressed due to low confidence (1)
packages/cloudflare-auth/src/cloudflare_auth/validators.py:268
- After replacing the blanket exception handler, token-controlled validation failures that are not
InvalidTokenErrorcan escape this method. For example,PyJWKClientErrorfromget_signing_key_from_jwt()(unknownkid/JWKS issues) or PydanticValidationErrorfromCloudflareJWTClaims(**payload)will bypass theValueErrorpath that the middleware handles and become a 500 instead of an authentication failure.
logger.warning("JWT validation failed: %s", str(e))
msg = "Token validation failed"
raise ValueError(msg) from e
def _validate_required_claims(self, payload: dict[str, Any]) -> None:
"""Validate that required claims are present.
Comment on lines
193
to
+195
| ) | ||
|
|
||
| # Validate required claims | ||
| # Validate required claims (defence-in-depth on top of "require") |
Comment on lines
+193
to
197
| except GCSNotFoundError: | ||
| raise | ||
| except (GoogleCloudError, OSError, ValueError) as e: | ||
| msg = f"Failed to access bucket '{self.bucket_name}': {e}" | ||
| raise GCSAuthError(msg) from e |
| # traversal sequences -- defence in depth. | ||
| try: | ||
| gcs_path = self._sanitize_gcs_path( | ||
| f"{gcs_prefix.rstrip('/')}/{rel_path}" |
Comment on lines
+281
to
+284
| # Reject ``..`` as a path segment to block traversal. We check | ||
| # segments rather than substring so legitimate names like | ||
| # ``v1..1`` (no slash) are still rejected (safer default) while | ||
| # ``my..file`` callers get a clear error and can rename. |
| jobs: | ||
| security: | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-security-analysis.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-security-analysis.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
| sbom: | ||
| name: SBOM & Security | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-sbom.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-sbom.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
| # Skip on forks (no PR comment permissions) | ||
| if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-mutation.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-mutation.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
| jobs: | ||
| fips-check: | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-fips-compatibility.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-fips-compatibility.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
| name: SLSA Level 3 | ||
| needs: [build] | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-slsa.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-slsa.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
| jobs: | ||
| reuse: | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-reuse.yml@main | ||
| uses: ByronWilliamsCPA/.github/.github/workflows/python-reuse.yml@e8fc83c98c2971ad1ece71573d28171463e30c16 # main |
JWT validator: - ``validate_token`` now refuses to run when ``cloudflare_audience_tag`` is unset rather than silently disabling audience verification, so a deployment with team domain set but no audience tag can no longer accept any token from that issuer. - Decode options for ``verify_aud`` / ``verify_iss`` are now unconditionally True (the new precondition makes the boolean toggle unnecessary). - Add specific handlers for ``PyJWKClientError`` (raised by ``get_signing_key_from_jwt`` on unknown ``kid`` / JWKS failures) and Pydantic ``ValidationError`` (raised by ``CloudflareJWTClaims(**payload)`` on malformed claims). Both now surface as ``ValueError`` so the middleware returns 401 instead of 500. GCS client: - Widen the bucket-access except clause to include ``google.auth.exceptions.GoogleAuthError`` so ``DefaultCredentialsError`` / ``RefreshError`` continue to be wrapped as ``GCSAuthError`` after the BLE001 cleanup. - ``upload_directory`` now joins ``rel_path.as_posix()`` before re-sanitizing the GCS path, so directory uploads from Windows do not feed backslashes into the sanitizer (which rejects them). - Update the ``..`` rejection comment to describe the actual stricter behaviour (segment check AND substring check). Workflows -- move elevated permissions off workflow scope onto the specific jobs that need them so the workflow-level scope can stay at ``contents: read``: - ``fips-compatibility.yml`` / ``mutation-testing.yml``: ``pull-requests: write`` moved to job. - ``sbom.yml`` / ``security-analysis.yml``: ``security-events: write`` (and ``pull-requests: write`` for security-analysis) moved to job. - ``reuse.yml``: replace ``permissions: read-all`` with explicit ``contents: read``. - ``slsa-provenance.yml``: drop workflow-scope ``contents: write``, ``id-token: write``, ``actions: read``, ``attestations: write``; the ``build`` job now carries the attestation permissions and the ``slsa`` job already has its own permissions block. Tests: - Add tests for the new "audience required" precondition and for the ``PyJWKClientError`` / Pydantic ``ValidationError`` -> ``ValueError`` mapping. https://claude.ai/code/session_01LfuLSQf3rcbYG1QdgL92a5
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security-focused hardening pass covering JWT auth, GCS utilities, GitHub Actions, and dependency posture. No new features — only tightened defaults and removed footguns.
JWT validator —
packages/cloudflare-auth/src/cloudflare_auth/validators.pyRS*/ES*/PS*) at constructor and against the unverified token header.noneand any HS* are rejected to block algorithm-confusion and signature-stripping attacks.jwt.decodeoptions are now spelled out and non-bypassable:verify_signature,verify_exp,verify_nbf,verify_iat, plus arequireset forexp/iat/iss/sub/aud. 30s leeway is allowed for clock skew.verify_exp=Falsekeyword fromvalidate_token/validate_token_async— no caller used it and keeping it was a footgun. Callsites inmiddleware.py/middleware_enhanced.pyalready pass positionally.except Exceptionwith PyJWT-specific handlers and anInvalidTokenErrorcatch-all so programmer errors are no longer mapped toValueError("Token validation failed").get_unverified_claimsnow logs a warning on every call so production misuse is visible.GCS client —
packages/gcs-utilities/src/gcs_utilities/client.py_sanitize_gcs_pathnow rejects control characters, backslashes, and./..as path segments (plus any..substring)._validate_local_pathrejects null bytes in local paths.upload_directoryre-sanitizes the joined path so symlinks / unusual filenames in the walked tree cannot reintroduce traversal sequences.delete_directoryrejects empty / whitespace-only prefixes — empty prefixes would otherwise match every object in the bucket.except Exceptionin_get_or_create_bucketreplaced with the specificGoogleCloudError/OSError/ValueErrorset.GitHub Actions —
.github/workflows/*.ymlByronWilliamsCPA/.github/...@mainreusable-workflow references pinned to commite8fc83c98c2971ad1ece71573d28171463e30c16.permissions:reduced tocontents: read; elevated rights moved to the specific jobs that need them (release.release,docs.docs,scorecard.scorecard,dependency-review,publish-artifact-registry.publish).step-security/harden-runner@a5ad31d6...withegress-policy: auditadded to every job with inline steps that lacked it: 5 jobs inci.yml, 2 inrelease.yml, plusdependency-review.yml,publish-artifact-registry.yml, andcodecov.yml's failure reporter.codeql.yml,slsa-provenance.yml, andpr-validation.ymlalready had it.Dependencies —
SECURITY-NOTES.md(new)pyproject.tomlandpackages/*/pyproject.tomluses an unbounded>=floor (no<X.0.0ceiling).pyjwt— sole auth gate; PyJWT has shipped behaviour-changing point releases (e.g. CVE-2022-29217).cryptography— substrate for JWT signature verification and TLS; FIPS compatibility tracks the OpenSSL build.google-cloud-storage— entire data plane forGCSClient; ADC / credential-resolution defaults have shifted across minors.Test plan
ruff checkclean on edited filesbasedpyright0 errors on edited filespytest packages/cloudflare-auth/tests/)yaml.safe_loadnone/HS256constructor rejection)delete_directoryrejects empty / whitespace / traversal prefixese8fc83c...) matches the desired pinned state forByronWilliamsCPA/.github<X.0.0upper bounds fromSECURITY-NOTES.mdin this PR or a follow-uphttps://claude.ai/code/session_01LfuLSQf3rcbYG1QdgL92a5
Generated by Claude Code