fix(license,oidc): fail-closed licensing + OIDC secret encryption, gate provider mutations (OM5/OM2/OM3)#60
Open
PenguinzTech wants to merge 1 commit into
Open
Conversation
…ate provider mutations (OM5/OM2/OM3) - OM5: require_premium no longer unlocks all features when RELEASE_MODE is unset. Adds LICENSE_ENFORCED (True in ProductionConfig); when enforced, the dev unlock path is disabled and a misconfigured/unreachable license client fails CLOSED (403). - OM2: OIDC client-secret encryption no longer silently returns plaintext/ciphertext on error — _encrypt/_decrypt raise; callers handle. Uses a dedicated OIDC_SECRET_ENCRYPTION_KEY (required in prod via validate()), not the JWT SECRET_KEY. TODO left for enterprise external KMS. - OM3: update_provider and delete_provider now premium-gated like create_provider (SSO/OIDC provider management is enterprise-tier), keeping @require_scope(system:admin). Tests: 12 new licensing/encryption/gating tests; suite 423 passed, 90.49% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideHardens licensing behavior to fail-closed in production and strengthens OIDC client-secret handling by enforcing dedicated encryption keys and consistent error handling, while premium-gating all OIDC provider mutation endpoints and adding targeted tests. Sequence diagram for OIDC secret encryption/decryption with dedicated keysequenceDiagram
participant Client as Client
participant FlaskApp as FlaskApp
participant OIDC as oidc_module
participant Config as Config
participant Fernet as Fernet
participant OAuthClient as AsyncOAuth2Client
Client->>FlaskApp: POST /oidc/providers (create_provider)
FlaskApp->>OIDC: _encrypt_secret(client_secret)
OIDC->>OIDC: _get_oidc_encryption_key()
OIDC->>Config: read OIDC_SECRET_ENCRYPTION_KEY, LICENSE_ENFORCED, SECRET_KEY
Config-->>OIDC: encryption_key or error
alt valid_encryption_key
OIDC->>Fernet: new Fernet(fernet_key)
OIDC->>Fernet: encrypt(plaintext)
Fernet-->>OIDC: ciphertext
OIDC-->>FlaskApp: encrypted_secret
FlaskApp->>FlaskApp: insert provider with client_secret_encrypted
FlaskApp-->>Client: 201 created
else encryption_error
OIDC-->>FlaskApp: raise ValueError
FlaskApp->>FlaskApp: logger.error Failed to encrypt client secret
FlaskApp-->>Client: 400 Failed to encrypt client secret
end
Client->>FlaskApp: GET /oidc/authorize (authorize)
FlaskApp->>OIDC: _decrypt_secret(provider.client_secret_encrypted)
OIDC->>OIDC: _get_oidc_encryption_key()
alt valid_encryption_key
OIDC->>Fernet: new Fernet(fernet_key)
OIDC->>Fernet: decrypt(ciphertext)
Fernet-->>OIDC: plaintext
OIDC-->>FlaskApp: client_secret
FlaskApp->>OAuthClient: AsyncOAuth2Client(client_id, client_secret,...)
OAuthClient-->>FlaskApp: authorization_url
FlaskApp-->>Client: redirect to IdP
else decryption_error
OIDC-->>FlaskApp: raise ValueError
FlaskApp->>FlaskApp: logger.error Failed to decrypt provider secret
FlaskApp-->>Client: 500 Provider configuration error
end
Flow diagram for updated require_premium licensing enforcementflowchart LR
A[request hits premium_gated_endpoint] --> B{LICENSE_ENFORCED}
B -- false --> C[call wrapped endpoint func]
B -- true --> D{_is_premium_bypassed}
D -- true --> C
D -- false --> E[call penguin_licensing.get_license_client]
E --> F{client.has_tier professional}
F -- true --> C
F -- false --> G[return 403 premium_features_locked]
E -. exception .-> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The repeated try/except blocks around
_decrypt_secretinauthorize,callback, andinitiate_link_identity(including logging and standardized 500 responses) could be consolidated into a small helper to reduce duplication and keep behavior consistent. - Several logging calls that handle failures (
OIDC secret encryption failed,OIDC secret decryption failed,OIDC authorize error for {provider_slug}) drop the underlying exception details; consider usinglogger.exceptionor including the exception in structured logging (at least at debug level) to make diagnosing production issues easier while still avoiding leaking secrets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated try/except blocks around `_decrypt_secret` in `authorize`, `callback`, and `initiate_link_identity` (including logging and standardized 500 responses) could be consolidated into a small helper to reduce duplication and keep behavior consistent.
- Several logging calls that handle failures (`OIDC secret encryption failed`, `OIDC secret decryption failed`, `OIDC authorize error for {provider_slug}`) drop the underlying exception details; consider using `logger.exception` or including the exception in structured logging (at least at debug level) to make diagnosing production issues easier while still avoiding leaking secrets.
## Individual Comments
### Comment 1
<location path="services/flask-backend/app/licensing.py" line_range="87-88" />
<code_context>
"penguin-licensing not available; denying premium access"
) # pragma: no cover
except Exception as e: # pragma: no cover
- logger.error(f"License check failed: {e}") # pragma: no cover
+ logger.error("License check failed; denying premium access") # pragma: no cover
return (
</code_context>
<issue_to_address>
**suggestion:** License check failure logs no exception details, which makes production diagnosis harder.
The previous log included the exception message; now it’s fully generic. For this critical premium-access gate, please retain minimal exception context in the log (e.g., `logger.error("License check failed; denying premium access: %s", e)`), while still ensuring no sensitive data is exposed to users.
```suggestion
except Exception as e: # pragma: no cover
logger.error("License check failed; denying premium access: %s", e) # pragma: no cover
```
</issue_to_address>
### Comment 2
<location path="services/flask-backend/app/oidc.py" line_range="200-201" />
<code_context>
- return plaintext
+ except ValueError:
+ raise
+ except Exception as e:
+ logger.error(f"OIDC secret encryption failed")
+ raise ValueError("Failed to encrypt OIDC client secret") from e
</code_context>
<issue_to_address>
**suggestion:** Encryption failure logging omits the underlying exception details, limiting observability.
The failure path is correct, but the log line drops the underlying exception `e`. Please include the exception details (e.g., via `logger.error("OIDC secret encryption failed: %s", e)`) so operational teams can diagnose key/config issues without changing the raised `ValueError` behavior.
Suggested implementation:
```python
except ValueError:
raise
except Exception as e:
logger.error(f"OIDC secret encryption failed")
raise ValueError("Failed to encrypt OIDC client secret") from e
```
```python
except Exception as e:
logger.error("OIDC secret encryption failed: %s", e)
raise ValueError("Failed to encrypt OIDC client secret") from e
```
</issue_to_address>
### Comment 3
<location path="services/flask-backend/tests/test_licensing_hardening.py" line_range="204" />
<code_context>
+@pytest.mark.asyncio
+async def test_decrypt_secret_never_returns_ciphertext_as_plaintext(app) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** The assertion inside `pytest.raises` is unreachable and can be simplified
Within `with pytest.raises(ValueError):`, any code after `_decrypt_secret(bad_ciphertext)` is never executed if the exception is raised, so `result` is unused and `assert result != bad_ciphertext` is dead code.
To clarify the test’s intent, keep only `_decrypt_secret(bad_ciphertext)` inside `pytest.raises`, or move assertions that should run when no exception is raised outside that context (e.g., via a separate test or a different control flow).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
87
to
+88
| except Exception as e: # pragma: no cover | ||
| logger.error(f"License check failed: {e}") # pragma: no cover | ||
| logger.error("License check failed; denying premium access") # pragma: no cover |
There was a problem hiding this comment.
suggestion: License check failure logs no exception details, which makes production diagnosis harder.
The previous log included the exception message; now it’s fully generic. For this critical premium-access gate, please retain minimal exception context in the log (e.g., logger.error("License check failed; denying premium access: %s", e)), while still ensuring no sensitive data is exposed to users.
Suggested change
| except Exception as e: # pragma: no cover | |
| logger.error(f"License check failed: {e}") # pragma: no cover | |
| logger.error("License check failed; denying premium access") # pragma: no cover | |
| except Exception as e: # pragma: no cover | |
| logger.error("License check failed; denying premium access: %s", e) # pragma: no cover |
Comment on lines
+200
to
+201
| except Exception as e: | ||
| logger.error(f"OIDC secret encryption failed") |
There was a problem hiding this comment.
suggestion: Encryption failure logging omits the underlying exception details, limiting observability.
The failure path is correct, but the log line drops the underlying exception e. Please include the exception details (e.g., via logger.error("OIDC secret encryption failed: %s", e)) so operational teams can diagnose key/config issues without changing the raised ValueError behavior.
Suggested implementation:
except ValueError:
raise
except Exception as e:
logger.error(f"OIDC secret encryption failed")
raise ValueError("Failed to encrypt OIDC client secret") from e except Exception as e:
logger.error("OIDC secret encryption failed: %s", e)
raise ValueError("Failed to encrypt OIDC client secret") from e| with pytest.raises(ValueError): | ||
| result = _decrypt_secret(bad_ciphertext) | ||
| # Should never reach here and return plaintext | ||
| assert result != bad_ciphertext |
There was a problem hiding this comment.
suggestion (testing): The assertion inside pytest.raises is unreachable and can be simplified
Within with pytest.raises(ValueError):, any code after _decrypt_secret(bad_ciphertext) is never executed if the exception is raised, so result is unused and assert result != bad_ciphertext is dead code.
To clarify the test’s intent, keep only _decrypt_secret(bad_ciphertext) inside pytest.raises, or move assertions that should run when no exception is raised outside that context (e.g., via a separate test or a different control flow).
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
1 participant
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.
Security fixes (Track A5a) — licensing + OIDC secret/provider hardening
Stacked on #57 (base =
fix/oidc-hardening). Targetsrelease/v0.1.x.require_premiumfell OPEN: it unlocked all premium features wheneverRELEASE_MODEwas unset (the default), so a prod deploy that forgot the flag gave everything away. AddsLICENSE_ENFORCED(True in ProductionConfig); when enforced, the dev unlock path is off and a misconfigured/unreachable license client fails closed (403). Domain bypass (configAPP_DOMAIN, never request Host) preserved.SECRET_KEYas the Fernet key. Now_encrypt_secret/_decrypt_secretraise on failure (never store/return unencrypted), using a dedicatedOIDC_SECRET_ENCRYPTION_KEY(required in prod). Callers handle the error. TODO left for enterprise external KMS.update_provideranddelete_providerare now@require_premiumlikecreate_provider(SSO/OIDC provider mgmt is Enterprise-tier), keeping@require_scope(system:admin).Tests
12 new tests (fail-closed license, encryption raises, provider gating). Suite: 423 passed, 90.49% coverage.
🤖 Generated with Claude Code
Summary by Sourcery
Harden licensing and OIDC secret handling to fail closed in production, and gate all OIDC provider mutations behind premium licensing.
Bug Fixes:
Enhancements:
Tests: