Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .github/workflows/jules-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
name: jules-review

# Optional: auto-invoke Jules for a security-focused review on every PR.
# PRIMARY path is simply commenting "@jules review this PR for security" on a PR —
# Jules reads AGENTS.md + .github/agents/security-reviewer.agent.md and responds.
# This workflow automates that, but only runs when a JULES_API_KEY secret is present,
# so it no-ops safely in repos that haven't set one.

on:
pull_request:
types: [opened, synchronize, ready_for_review]

permissions:
contents: read
pull-requests: write

jobs:
jules:
runs-on: ubuntu-latest
steps:
- name: Guard — only run when a Jules key is configured
id: guard
run: |
if [ -n "${{ secrets.JULES_API_KEY }}" ]; then
echo "enabled=true" >> "$GITHUB_OUTPUT"
else
echo "enabled=false" >> "$GITHUB_OUTPUT"
echo "No JULES_API_KEY set — skipping automated Jules review. Use @jules on the PR instead."
fi
- name: Jules security review
if: steps.guard.outputs.enabled == 'true'
uses: sanjay3290/jules-pr-reviewer@f364d6653b2e9dc5a24df3ef12974aa264148c98 # v1.0.1
with:
jules_api_key: ${{ secrets.JULES_API_KEY }}
github_token: ${{ github.token }}
extra_instructions: >
Review this pull request as an adversarial application-security reviewer.
Follow .github/agents/security-reviewer.agent.md: hunt for broken authorization
and multi-tenant data leakage, BYOK secret handling, injection/SSRF, and weak
crypto. Default to "this is a finding" when unsure. Cite file:line and propose the fix.
28 changes: 28 additions & 0 deletions internal/canary/billing.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package canary

import "log"

// CANARY — deliberate defects to verify the review standard fires. DO NOT MERGE.

// hardcoded credential
const billingToken = "b7f3d91e4c2a8056f1d3e7a94c0b2856d4f9a1e3"
Comment on lines +7 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚨 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.

Comment on lines +7 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 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


type Invoice struct {
ID string
OwnerID string
Amount int
}

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 {
Comment on lines +18 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚨 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.

log.Printf("billing lookup id=%s key=%s", id, billingToken) // secret in log

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚨 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 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.

Suggested change
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.

for i := range invoices {
if invoices[i].ID == id {
return &invoices[i]
Comment on lines +18 to +24

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 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.

}
}
return nil
}
Loading