Fix logic-path audit defects (decode corrupted sources + correctness bugs) - #32
canstralian wants to merge 3 commits into
Conversation
…ipts Decode base64-corrupted source files (mcp server, scope_mapper, import script, vector registry) so they lint and run as intended, and fix deterministic correctness bugs surfaced by the logic-path audit: - import_vectors.sh: fix unexpanded $EXECUTE guard, wrong URL parameter expansion, missing error handling, malformed dry-run quoting; add set -euo pipefail and URL scheme validation - scope_mapper: fix __main__ guard typo, source base/table from env - storage/pipeline Flask apps: bind SQLAlchemy via db.init_app so models are usable; add report timestamp - dashboard db: replace non-null assertion on DATABASE_URL with explicit check - recon route: validate body with existing zod schema, guard URL parsing, wrap handler in try/catch - hooks: drop unused zustand imports - remove unused imports from nlp_processor https://claude.ai/code/session_01XXDAcaDMRNh1s6fohSbkKq
|
Warning Review limit reached
More reviews will be available in 54 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR decodes previously obfuscated shell scripts and configuration files, fixing critical bugs in ChangesInfrastructure Setup and Stabilization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the BugBountyOS by replacing hardcoded configuration values with environment variables in the Airtable adapter, enhancing the vector import script with validation and error handling, and adding input validation to the dashboard's recon routes. It also configures SQLAlchemy for the pipeline and storage services and removes unused imports. Feedback suggests removing hardcoded default IDs to improve security, making the git branch name configurable in the import script, and masking raw error messages in API responses to prevent information disclosure.
| const { target, scanType } = parsed.data; | ||
| res.json(await performRecon(target, scanType)); | ||
| } catch (err) { | ||
| res.status(500).json({ error: err instanceof Error ? err.message : "scan failed" }); |
There was a problem hiding this comment.
Exposing the raw error message (err.message) to the client in a 500 response can lead to information disclosure, potentially revealing internal system details. It is recommended to log the error details server-side and return a generic error message to the client.
console.error("Scan failed:", err);
res.status(500).json({ error: "scan failed" });
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane/registry/vectors.yaml (1)
1-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore base64 encoding for this registry file.
This file is checked in as plain YAML, but this path is required to be stored base64-encoded in this repository.
As per coding guidelines, "Use base64 encoding for: ...
control-plane/registry/vectors.yaml...".🤖 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 `@control-plane/registry/vectors.yaml` around lines 1 - 29, This file was checked in as plain YAML but needs to be stored base64-encoded; replace the current plain-text content of control-plane/registry/vectors.yaml with the base64 encoding of the entire YAML content (the full "vectors:" document including all entries like ids dashboard, pipeline, storage, red-sage and their fields), ensuring you encode using standard base64 (UTF‑8 input) and commit the encoded string in place of the plain YAML so the repository follows the guideline to store control-plane/registry/vectors.yaml as base64.
🤖 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 `@adapters/mcp/server.py`:
- Around line 7-10: The check_scope function currently ignores its asset_id
parameter and always returns a permissive string; replace the stub in
check_scope with real authorization logic that validates the given asset_id
against the BugBountyOS Immune System (e.g., call an existing immune system
client method like immune_system_client.verify_asset(asset_id) or query the
scope datastore), return a clear authorization result (authorized/denied or
raise a specific exception) based on that verification, and handle/log any
errors from the external call so out-of-scope assets are not permitted by
default.
In `@import_vectors.sh`:
- Line 31: The dry-run echo currently prefixes the command with "[DRY-RUN]"
which makes it non-runnable; change the behavior in the import_vectors.sh line
that echoes the command (the echo of "git subtree add --prefix='vectors/$NAME'
'$URL' main --squash") so it prints a directly copy-pastable command — for
example emit the raw command string without the "[DRY-RUN]" token or print the
token as a separate, non-interfering comment and then echo the executable
command itself so users can copy-paste and run the shown command.
In `@vectors/dashboard/server/routes/recon.ts`:
- Around line 57-62: The current parsing of the incoming target in recon.ts
attempts new URL(target) then falls back to new URL(`https://${target`) but if
both fail it bubbles to a 500; change this to treat an unparseable target as a
client error by returning a 400. In the recon route handler, update the
try/catch logic around the url and target variables so that if both attempts to
construct URL (new URL(target) and new URL(`https://${target}`)) throw, you call
res.status(400).json(...) (or next with a 400 HTTP error) with a clear
validation message about the invalid target instead of allowing the error to
propagate to a 500. Ensure the code references the same url/target variables so
downstream logic still uses the validated URL.
- Around line 76-77: The catch block in the recon route currently sends
err.message to the client; instead, stop exposing internal error text by
returning a stable generic message (e.g., res.status(500).json({ error:
"Internal server error" }) or { error: "scan failed" }) and log the full error
server-side (e.g., console.error(err) or use the existing logger) before sending
the response; update the catch that references err and res.status(500).json(...)
to perform server-side logging of err and return the generic message to the
client.
---
Outside diff comments:
In `@control-plane/registry/vectors.yaml`:
- Around line 1-29: This file was checked in as plain YAML but needs to be
stored base64-encoded; replace the current plain-text content of
control-plane/registry/vectors.yaml with the base64 encoding of the entire YAML
content (the full "vectors:" document including all entries like ids dashboard,
pipeline, storage, red-sage and their fields), ensuring you encode using
standard base64 (UTF‑8 input) and commit the encoded string in place of the
plain YAML so the repository follows the guideline to store
control-plane/registry/vectors.yaml as base64.
🪄 Autofix (Beta)
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
Run ID: 4cf8d572-77f3-4145-8421-ef19a488be92
📒 Files selected for processing (13)
adapters/airtable/scope_mapper.pyadapters/mcp/server.pycontrol-plane/registry/vectors.yamlimport_vectors.shvectors/dashboard/client/src/hooks/use-programs.tsvectors/dashboard/client/src/hooks/use-toast.tsvectors/dashboard/client/src/hooks/use-user.tsvectors/dashboard/db/index.tsvectors/dashboard/server/routes/recon.tsvectors/pipeline/app.pyvectors/pipeline/models.pyvectors/pipeline/nlp_processor.pyvectors/storage/app.py
💤 Files with no reviewable changes (3)
- vectors/dashboard/client/src/hooks/use-programs.ts
- vectors/dashboard/client/src/hooks/use-user.ts
- vectors/dashboard/client/src/hooks/use-toast.ts
| def check_scope(asset_id: str) -> str: | ||
| """Query the BugBountyOS Immune System to verify if an asset is authorized.""" | ||
| return "Importing Airtable Adapter... Currently Permissive mode." | ||
|
|
There was a problem hiding this comment.
check_scope ignores input and always returns permissive authorization.
This currently bypasses the intended scope check path and can authorize out-of-scope assets by behavior.
Suggested fix
`@mcp.tool`()
def check_scope(asset_id: str) -> str:
"""Query the BugBountyOS Immune System to verify if an asset is authorized."""
- return "Importing Airtable Adapter... Currently Permissive mode."
+ # Wire to real adapter/policy check; deny by default if unavailable.
+ authorized = False
+ return "authorized" if authorized else "unauthorized"🤖 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 `@adapters/mcp/server.py` around lines 7 - 10, The check_scope function
currently ignores its asset_id parameter and always returns a permissive string;
replace the stub in check_scope with real authorization logic that validates the
given asset_id against the BugBountyOS Immune System (e.g., call an existing
immune system client method like immune_system_client.verify_asset(asset_id) or
query the scope datastore), return a clear authorization result
(authorized/denied or raise a specific exception) based on that verification,
and handle/log any errors from the external call so out-of-scope assets are not
permitted by default.
| exit 1 | ||
| } | ||
| else | ||
| echo "[DRY-RUN] git subtree add --prefix='vectors/$NAME' '$URL' main --squash" |
There was a problem hiding this comment.
Dry-run output is not directly runnable due to the [DRY-RUN] prefix.
The emitted line includes a non-command token at the start, so copy-paste execution fails.
Suggested fix
- echo "[DRY-RUN] git subtree add --prefix='vectors/$NAME' '$URL' main --squash"
+ echo "git subtree add --prefix='vectors/$NAME' '$URL' main --squash"📝 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.
| echo "[DRY-RUN] git subtree add --prefix='vectors/$NAME' '$URL' main --squash" | |
| echo "git subtree add --prefix='vectors/$NAME' '$URL' main --squash" |
🤖 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 `@import_vectors.sh` at line 31, The dry-run echo currently prefixes the
command with "[DRY-RUN]" which makes it non-runnable; change the behavior in the
import_vectors.sh line that echoes the command (the echo of "git subtree add
--prefix='vectors/$NAME' '$URL' main --squash") so it prints a directly
copy-pastable command — for example emit the raw command string without the
"[DRY-RUN]" token or print the token as a separate, non-interfering comment and
then echo the executable command itself so users can copy-paste and run the
shown command.
| let url: URL; | ||
| try { | ||
| url = new URL(target); | ||
| } catch { | ||
| url = new URL(`https://${target}`); | ||
| } |
There was a problem hiding this comment.
Return 400 for unparseable target instead of bubbling to 500.
If both URL parses fail, this is still client input error, but it currently falls through to the generic 500 path. Normalize/validate target here (or in schema) and surface it as a bad-request failure.
Suggested fix
async function performRecon(target: string, scanType: string): Promise<ScanResult> {
let url: URL;
try {
url = new URL(target);
} catch {
- url = new URL(`https://${target}`);
+ try {
+ url = new URL(`https://${target}`);
+ } catch {
+ throw new Error("invalid target");
+ }
}
const domain = url.hostname;
return { domain, timestamp: new Date().toISOString(), whois: null, dns: { a: [], mx: [], ns: [], txt: [] }, ports: [], technologies: [] };
}📝 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.
| let url: URL; | |
| try { | |
| url = new URL(target); | |
| } catch { | |
| url = new URL(`https://${target}`); | |
| } | |
| let url: URL; | |
| try { | |
| url = new URL(target); | |
| } catch { | |
| try { | |
| url = new URL(`https://${target}`); | |
| } catch { | |
| throw new Error("invalid target"); | |
| } | |
| } |
🤖 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 `@vectors/dashboard/server/routes/recon.ts` around lines 57 - 62, The current
parsing of the incoming target in recon.ts attempts new URL(target) then falls
back to new URL(`https://${target`) but if both fail it bubbles to a 500; change
this to treat an unparseable target as a client error by returning a 400. In the
recon route handler, update the try/catch logic around the url and target
variables so that if both attempts to construct URL (new URL(target) and new
URL(`https://${target}`)) throw, you call res.status(400).json(...) (or next
with a 400 HTTP error) with a clear validation message about the invalid target
instead of allowing the error to propagate to a 500. Ensure the code references
the same url/target variables so downstream logic still uses the validated URL.
| } catch (err) { | ||
| res.status(500).json({ error: err instanceof Error ? err.message : "scan failed" }); |
There was a problem hiding this comment.
Avoid exposing raw internal error messages in HTTP 500 responses.
err.message should not be sent directly to clients. Return a stable generic message (and log full error server-side) to reduce information leakage.
Suggested fix
} catch (err) {
- res.status(500).json({ error: err instanceof Error ? err.message : "scan failed" });
+ // log err internally
+ res.status(500).json({ error: "scan failed" });
}📝 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.
| } catch (err) { | |
| res.status(500).json({ error: err instanceof Error ? err.message : "scan failed" }); | |
| } catch (err) { | |
| // log err internally | |
| res.status(500).json({ error: "scan failed" }); | |
| } |
🤖 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 `@vectors/dashboard/server/routes/recon.ts` around lines 76 - 77, The catch
block in the recon route currently sends err.message to the client; instead,
stop exposing internal error text by returning a stable generic message (e.g.,
res.status(500).json({ error: "Internal server error" }) or { error: "scan
failed" }) and log the full error server-side (e.g., console.error(err) or use
the existing logger) before sending the response; update the catch that
references err and res.status(500).json(...) to perform server-side logging of
err and return the generic message to the client.
2 participants
Addresses the deterministic, unambiguous findings from the logic-path audit (issues #24–#31). Design-requiring work is intentionally left for follow-up (see bottom).
Root cause: base64-corrupted source files
Four files were stored base64-encoded on disk, so they are invalid as Python/shell/YAML —
ruff check .andshellcheckwould error on them in CI. Decoded to plaintext:adapters/mcp/server.pyadapters/airtable/scope_mapper.pyimport_vectors.shcontrol-plane/registry/vectors.yamlFixes by issue
import_vectors.sh—if [ "EXECUTE" -eq 1 ]→"$EXECUTE"(live mode never triggered);${entry#:*}→${entry#*:}(URL retained thename:prefix); addedset -euo pipefail,https://scheme validation, error handling aroundgit subtree add, and fixed the malformed dry-run quoting so the echoed command is copy-pasteable.scope_mapper.py— fixed__main_→__main__guard typo;base_id/table now read fromAIRTABLE_BASE_ID/AIRTABLE_SCOPE_TABLEenv with the prior value as default.db.init_app(app)and setSQLALCHEMY_DATABASE_URIsofrom app import dbresolves and models are usable; addedcreated_attoBugReport.db/index.ts— replacedprocess.env.DATABASE_URL!non-null assertion with an explicit check that throws a clear "DATABASE_URL is not set" error at startup.recon.ts— validatereq.bodywith the (previously unused)scanRequestSchemareturning 400 on failure; guardnew URL(target)against non-URL input; wrap the handler in try/catch.zustandimports that falsely signalled a wired-up store.nlp_processor.py— removed unused imports.Verified locally
ruff check adapters vectors→ all checks passedbash -n import_vectors.sh+ dry-run produces correct, runnable outputpython3 -m py_compileon the decoded Python filesIntentionally deferred (need design, not just bug fixes)
check_scopestill returns the permissive string;is_authorizedstill deny-all) — needs a single authoritative, fail-closed scope source.scanConfigswiring depends on it.schema.tscolumn left as-is.statetransitions and thered-sagecontract_version: 0semantics (left unchanged — may be deliberate since red-sage istainted/pending).auth/mainblueprints define no routes yet) and Dashboard hooks collapse loading/error/empty states into one #29 real async hook state.Closes #27. Partially addresses #24, #25, #26, #29, #30, #31.
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor