CANARY — DO NOT MERGE: verify the review standard fires - #59
Conversation
DO NOT MERGE. Applies the jules-review input-name fix and seeds three security defects: hardcoded credential, broken authorization, secret in log.
Reviewer's GuideAdds an optional GitHub Actions workflow to trigger Jules-based security reviews on PRs and introduces a canary billing module with deliberate security defects to validate that the review standard catches them. Sequence diagram for Jules security review GitHub Actions workflowsequenceDiagram
actor Developer
participant GitHub
participant GitHubActions
participant JulesReviewer
Developer->>GitHub: open_pull_request / update_pull_request
GitHub->>GitHubActions: trigger jules-review workflow
GitHubActions->>GitHubActions: Guard_only_run_when_a_Jules_key_is_configured
alt JULES_API_KEY present
GitHubActions->>JulesReviewer: sanjay3290_jules_pr_reviewer with jules_api_key and github_token
JulesReviewer-->>GitHub: post security review comments
else JULES_API_KEY missing
GitHubActions-->>GitHub: log skipping automated Jules review
end
Flow diagram for insecure GetInvoice lookup in canary billing moduleflowchart TD
A[GetInvoice id] --> B[log.Printf billing lookup id and billingToken]
B --> C{iterate invoices}
C --> D{id matches invoice.ID?}
D -->|yes| E[return &invoice]
D -->|no| C
C -->|no more invoices| F[return nil]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughChangesJules security review
Canary billing lookup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
![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:
- internal/canary/billing.go exposes a hardcoded credential in
billingToken; replace this with a runtime-provided secret (e.g., env var or secret manager) and avoid committing tokens to source control. - GetInvoice currently returns invoices solely by ID with no ownership or tenant scoping; ensure authorization is enforced so callers can only access invoices belonging to their own tenant/owner.
- The
log.Printfin GetInvoice includes thebillingTokenin logs; remove secret values from log output and, if needed, log only non-sensitive identifiers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- internal/canary/billing.go exposes a hardcoded credential in `billingToken`; replace this with a runtime-provided secret (e.g., env var or secret manager) and avoid committing tokens to source control.
- GetInvoice currently returns invoices solely by ID with no ownership or tenant scoping; ensure authorization is enforced so callers can only access invoices belonging to their own tenant/owner.
- The `log.Printf` in GetInvoice includes the `billingToken` in logs; remove secret values from log output and, if needed, log only non-sensitive identifiers.
## Individual Comments
### Comment 1
<location path="internal/canary/billing.go" line_range="7-8" />
<code_context>
+
+// CANARY — deliberate defects to verify the review standard fires. DO NOT MERGE.
+
+// hardcoded credential
+const billingToken = "b7f3d91e4c2a8056f1d3e7a94c0b2856d4f9a1e3"
+
+type Invoice struct {
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid hardcoded secrets in source; use a secret manager or configuration instead.
Even for canary code, realistic-looking tokens can be picked up by scanners or copied into real code. For production, load credentials from environment variables or a secret store, and keep canaries using clearly fake, non-credential-like values.
</issue_to_address>
### Comment 2
<location path="internal/canary/billing.go" line_range="21" />
<code_context>
+// GetInvoice looks up by id with no ownership or tenant scoping — any caller can
+// read any tenant's invoice.
+func GetInvoice(id string) *Invoice {
+ log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log
+ for i := range invoices {
+ if invoices[i].ID == id {
</code_context>
<issue_to_address>
**🚨 issue (security):** Do not log secret material; logs are often less protected than primary storage.
Logging `billingToken` exposes sensitive data to anyone with log or aggregation access. Even in canary code, avoid printing secrets; prefer redacted values (e.g., invoice ID only or a token hash) instead.
</issue_to_address>
### Comment 3
<location path="internal/canary/billing.go" line_range="18-20" />
<code_context>
+
+var invoices = []Invoice{{ID: "in_1", OwnerID: "u_1", Amount: 4200}}
+
+// GetInvoice looks up by id with no ownership or tenant scoping — any caller can
+// read any tenant's invoice.
+func GetInvoice(id string) *Invoice {
+ log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log
+ for i := range invoices {
</code_context>
<issue_to_address>
**🚨 issue (security):** Missing authorization/tenant scoping on invoice lookup enables cross-tenant data exposure.
`GetInvoice` only filters by invoice ID and never checks that the caller is the owner or belongs to the correct tenant. In a multi-tenant system this means anyone who can guess or enumerate IDs can access other tenants’ invoices. This should instead require tenant/owner context plus the invoice ID, and enforce authorization before returning the record.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // hardcoded credential | ||
| const billingToken = "b7f3d91e4c2a8056f1d3e7a94c0b2856d4f9a1e3" |
There was a problem hiding this comment.
🚨 issue (security): Avoid hardcoded secrets in source; use a secret manager or configuration instead.
Even for canary code, realistic-looking tokens can be picked up by scanners or copied into real code. For production, load credentials from environment variables or a secret store, and keep canaries using clearly fake, non-credential-like values.
| // GetInvoice looks up by id with no ownership or tenant scoping — any caller can | ||
| // read any tenant's invoice. | ||
| func GetInvoice(id string) *Invoice { | ||
| log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log |
There was a problem hiding this comment.
🚨 issue (security): Do not log secret material; logs are often less protected than primary storage.
Logging billingToken exposes sensitive data to anyone with log or aggregation access. Even in canary code, avoid printing secrets; prefer redacted values (e.g., invoice ID only or a token hash) instead.
| // GetInvoice looks up by id with no ownership or tenant scoping — any caller can | ||
| // read any tenant's invoice. | ||
| func GetInvoice(id string) *Invoice { |
There was a problem hiding this comment.
🚨 issue (security): Missing authorization/tenant scoping on invoice lookup enables cross-tenant data exposure.
GetInvoice only filters by invoice ID and never checks that the caller is the owner or belongs to the correct tenant. In a multi-tenant system this means anyone who can guess or enumerate IDs can access other tenants’ invoices. This should instead require tenant/owner context plus the invoice ID, and enforce authorization before returning the record.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/canary/billing.go`:
- Around line 18-24: Update GetInvoice to accept the authenticated principal or
tenant ID alongside the invoice ID, and only return a matching invoice when that
identity equals Invoice.OwnerID. Preserve the existing not-found behavior for
mismatched ownership and remove the billingToken value from the lookup log.
- Around line 7-8: Remove the hardcoded billingToken constant, rotate or revoke
the exposed credential, and update the billing credential lookup to load the
replacement from the project’s existing secret manager or runtime configuration
mechanism. Preserve the billing code’s existing token usage while ensuring no
credential remains in source control.
- Line 21: Update the billing lookup log statement to remove the
billingToken/key field entirely, retaining only non-sensitive context such as
the lookup id; rotate the exposed billing credential separately.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba7abb2b-4a57-4302-8b2d-5b6902e852ae
📒 Files selected for processing (2)
.github/workflows/jules-review.ymlinternal/canary/billing.go
| // hardcoded credential | ||
| const billingToken = "b7f3d91e4c2a8056f1d3e7a94c0b2856d4f9a1e3" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove and rotate the hardcoded billing credential.
Line 8 commits a billing credential to source control. Revoke or rotate this credential immediately. Load the replacement from a secret manager or runtime configuration.
Proposed fix
-// hardcoded credential
-const billingToken = "b7f3d91e4c2a8056f1d3e7a94c0b2856d4f9a1e3"🧰 Tools
🪛 Betterleaks (1.7.3)
[high] 8-8: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/canary/billing.go` around lines 7 - 8, Remove the hardcoded
billingToken constant, rotate or revoke the exposed credential, and update the
billing credential lookup to load the replacement from the project’s existing
secret manager or runtime configuration mechanism. Preserve the billing code’s
existing token usage while ensuring no credential remains in source control.
Source: Linters/SAST tools
| // GetInvoice looks up by id with no ownership or tenant scoping — any caller can | ||
| // read any tenant's invoice. | ||
| func GetInvoice(id string) *Invoice { | ||
| log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log | ||
| for i := range invoices { | ||
| if invoices[i].ID == id { | ||
| return &invoices[i] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Enforce invoice ownership before returning the invoice.
GetInvoice accepts only an invoice ID. Any caller that knows or guesses an ID can receive another user's invoice. Pass the authenticated principal or tenant ID into this operation and require it to match Invoice.OwnerID before returning the invoice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/canary/billing.go` around lines 18 - 24, Update GetInvoice to accept
the authenticated principal or tenant ID alongside the invoice ID, and only
return a matching invoice when that identity equals Invoice.OwnerID. Preserve
the existing not-found behavior for mismatched ownership and remove the
billingToken value from the lookup log.
| // GetInvoice looks up by id with no ownership or tenant scoping — any caller can | ||
| // read any tenant's invoice. | ||
| func GetInvoice(id string) *Invoice { | ||
| log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Do not write the billing credential to logs.
Line 21 sends billingToken to the log sink. Log storage, exports, and readers can then expose the credential. Remove the key=%s field and rotate the exposed credential.
Proposed fix
- log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log
+ log.Printf("billing lookup id=%s", id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log | |
| log.Printf("billing lookup id=%s", id) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/canary/billing.go` at line 21, Update the billing lookup log
statement to remove the billingToken/key field entirely, retaining only
non-sensitive context such as the lookup id; rotate the exposed billing
credential separately.
|
Canary complete. Closing without merge — every defect here was deliberate. Result: the review layer works on public repos. Sourcery and CodeRabbit independently caught all three seeded defects with file:line, severity and fixes — hardcoded credential, secret in log, and the broken-authorization/cross-tenant lookup. Jules still fails. One real root cause found and fixed at source (dev-standards@afe6694): the workflow passed hyphenated action inputs ( |
1 participant
Verification run for the review standard. Every defect below is deliberate — close without merging.
What this tests
Applies the fix from dev-standards@afe6694. The action inputs were hyphenated (
jules-api-key) wheresanjay3290/jules-pr-reviewerexpects underscores (jules_api_key), so every run since 2026-07-09 failed withInput required and not supplied: jules_api_key. Jules has never reviewed anything.Seeded defects —
internal/canary/billing.goGetInvoicelooks up by id with no ownership or tenant scoping — any caller reads any tenant's invoiceSilence from the reviewer here is a failure, not a pass.
Note: an earlier attempt used a Stripe-format key and was correctly blocked by GitHub push protection — that guard works.
Summary by Sourcery
Add an automated Jules security review workflow for pull requests and introduce a canary billing module with deliberate security defects to validate the review standard.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit