feat: Add allowlist URI resolver for secure resource loading #558
Conversation
📝 WalkthroughWalkthroughAdds a Product Requirements Document and an implementation plan for an Allowlist URI Resolver, plus new public resolver APIs, denylist primitives, scheme policy, and package stubs to support rule-based allow/deny URI resolution and configuration-driven integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.claude/rules/development-workflow.md (2)
69-73: Add language specifiers to code blocks for proper syntax highlighting.Multiple code blocks are missing language specifiers, which affects rendering and syntax highlighting. Static analysis identified this at lines 69, 111, 129, and 191.
Apply these fixes:
-``` +```text /prds/mm-dd-yyy-name/ ├── plan.md # PRD + implementation plan └── (supporting docs) # Diagrams, research, screenshots, etc.-
+mermaid
Code Complete
↓
Code Review Agents (parallel) → Consolidated Issues Report-``` +```text GitHub issue/prompt ↓ brainstorming + writing-plans → /prds/mm-dd-yyy-name/plan.md-
+mermaid
Bug Report / Issue
↓
systematic-debugging + root-cause-tracingAlso applies to: 111-121, 129-143, 191-207
3-26: Consider balancing mandatory protocols with practical flexibility.The "Skill Usage Protocol (MANDATORY)" section uses very strong language ("STOP", "RED FLAGS", "Never"). While structure is valuable, overly prescriptive rules may:
- Hinder rapid prototyping and exploration
- Create friction for simple tasks
- Not account for context-specific needs
Consider rewording to:
- "Strongly recommended" instead of "MANDATORY"
- "Consider these patterns" instead of "If you think any of these, STOP"
- Allow developer judgment for simple/exploratory tasks
The workflow documentation is excellent, but giving developers trust and flexibility often yields better results than rigid mandates.
PRDs/20251217-allowlist-resolver/PRD.md (1)
83-116: Add language specifiers to improve diagram and code block rendering.Multiple code blocks and diagrams lack language specifiers. For ASCII diagrams, use
```textand for class hierarchies use```textor```plaintext.-``` +```text ┌─────────────────────────────────────────────────────────────────┐ │ IAllowlistUriResolver │-
+text
gov.nist.secauto.metaschema.core.model.resolver/
├── IAllowlistUriResolver.java # Main interface-``` +```text User Request (URI) │ ▼Also applies to: 120-138, 142-170 </blockquote></details> <details> <summary>PRDs/20251208-constraint-builder/PRD.md (1)</summary><blockquote> `115-126`: **Add language specifier for package structure diagram.** ```diff -``` +```text gov.nist.secauto.metaschema.core.testsupport.builder/ ├── IConstraintSetBuilder.java # Main builder interface</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b270e0af9610aaccab3244095f6c2a2b9790992d and ad43a4a86ceebe8a05d39b05ceb6eb694b811f92. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `.claude/rules/development-workflow.md` (1 hunks) * `.claude/settings.json` (1 hunks) * `PRDs/20251208-constraint-builder/PRD.md` (1 hunks) * `PRDs/20251208-constraint-builder/implementation-plan.md` (1 hunks) * `PRDs/20251217-allowlist-resolver/PRD.md` (1 hunks) * `PRDs/20251217-allowlist-resolver/implementation-plan.md` (1 hunks) * `core/.settings/org.eclipse.jdt.core.prefs` (1 hunks) * `databind/.settings/org.eclipse.jdt.core.prefs` (1 hunks) * `databind/src/main/java/gov/nist/secauto/metaschema/databind/IBindingContext.java` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.java</summary> **📄 CodeRabbit inference engine (CLAUDE.md)** > `**/*.java`: All code changes must follow the Javadoc style guide (docs/javadoc-style-guide.md). New code requires 100% Javadoc coverage on public/protected members. Modified code must add/update Javadoc on any members touched. All Javadoc must include @param, @return, @throws tags in the correct order (BLOCKING) > Java target version must be Java 11. Use SpotBugs annotations (@NonNull, @Nullable) for null safety in code. > Follow package naming convention gov.nist.secauto.metaschema.* for all Java packages > Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green Files: - `databind/src/main/java/gov/nist/secauto/metaschema/databind/IBindingContext.java` </details> </details><details> <summary>🧠 Learnings (7)</summary> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/*.java : Java target version must be Java 11. Use SpotBugs annotations (NonNull, Nullable) for null safety in code.**Applied to files:** - `core/.settings/org.eclipse.jdt.core.prefs` - `databind/.settings/org.eclipse.jdt.core.prefs` </details> <details> <summary>📚 Learning: 2025-12-13T21:16:12.281Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T21:16:12.281Z
Learning: Constraints are documented inwebsite/content/specification/syntax/constraints.md. When adding new constraint types, update the constraint type lists fordefine-flag,define-field, anddefine-assembly, and add a new section with syntax table and examples**Applied to files:** - `PRDs/20251208-constraint-builder/PRD.md` </details> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: For larger initiatives requiring multiple PRs, use structured PRD (Product Requirements Document) approach. PRDs stored in PRDs/-/ with PRD.md and implementation-plan.md. Target ≤50 files per PR, maximum 100 files per PR.**Applied to files:** - `.claude/rules/development-workflow.md` </details> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Code changes should use TDD principles: write tests first before implementing code, watch tests fail, write minimal code to pass tests, then refactor. For existing code without tests, add tests when modifying files to ensure behavioral equivalence.**Applied to files:** - `.claude/rules/development-workflow.md` </details> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: All PRs must be created from a personal fork and must target the develop branch (BLOCKING - required by CONTRIBUTING.md)**Applied to files:** - `.claude/rules/development-workflow.md` </details> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: All PRs require passing CI checks before merge**Applied to files:** - `.claude/rules/development-workflow.md` </details> <details> <summary>📚 Learning: 2025-12-17T13:27:43.669Z</summary>Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: Applies to **/*.java : Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green**Applied to files:** - `.claude/rules/development-workflow.md` </details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>PRDs/20251208-constraint-builder/PRD.md</summary> [style] ~163-~163: To elevate your writing, try using a synonym here. Context: ...-----| | Complex constraint hierarchies hard to express | Medium | Start with simple... (HARD_TO) </details> <details> <summary>PRDs/20251217-allowlist-resolver/PRD.md</summary> [grammar] ~5-~5: Use a hyphen to join words. Context: ...k. **Issue:** [#183 - Add new allowlist only resolver for loading models, instan... (QB_NEW_EN_HYPHEN) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>PRDs/20251208-constraint-builder/PRD.md</summary> 115-115: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>PRDs/20251217-allowlist-resolver/PRD.md</summary> 83-83: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 120-120: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 142-142: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>.claude/rules/development-workflow.md</summary> 69-69: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 111-111: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 129-129: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 191-191: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>PRDs/20251217-allowlist-resolver/implementation-plan.md</summary> 39-39: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 80-80: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 88-88: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 172-172: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 180-180: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 239-239: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 253-253: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 308-308: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 322-322: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 382-382: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 397-397: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 458-458: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 466-466: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 581-581: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 589-589: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 601-601: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 609-609: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 617-617: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 635-635: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 695-695: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 703-703: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 853-853: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 861-861: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 877-877: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 961-961: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 969-969: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1137-1137: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1145-1145: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1161-1161: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1215-1215: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1221-1221: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1325-1325: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1331-1331: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1347-1347: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1409-1409: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1432-1432: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1440-1440: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1448-1448: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1482-1482: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 1489-1489: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)</summary> * GitHub Check: Code * GitHub Check: Website </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>core/.settings/org.eclipse.jdt.core.prefs (1)</summary><blockquote> `115-115`: No annotation processors are used in this project. The codebase contains only a Maven plugin API `@Inject` annotation, which does not require compile-time processing. Disabling `processAnnotations` only affects Eclipse's editor-time compilation; Maven and Gradle builds handle annotation processing independently and are unaffected by this setting. </blockquote></details> <details> <summary>.claude/settings.json (1)</summary><blockquote> `1-2`: **LGTM - Valid placeholder configuration.** Empty JSON object is syntactically correct and serves as a placeholder for future Claude configuration settings. </blockquote></details> <details> <summary>databind/src/main/java/gov/nist/secauto/metaschema/databind/IBindingContext.java (1)</summary><blockquote> `501-502`: **LGTM - Formatting improvement enhances readability.** The blank line appropriately separates variable declaration from subsequent initialization, improving code clarity. </blockquote></details> <details> <summary>PRDs/20251217-allowlist-resolver/PRD.md (1)</summary><blockquote> `1-410`: **Excellent security-focused PRD with comprehensive threat modeling.** This PRD demonstrates strong security architecture with: - Clear threat model addressing LFI, SSRF, XXE, and metadata endpoint attacks - Defense-in-depth via user allowlist + non-configurable denylist - Well-designed hierarchical configuration model - Comprehensive built-in denylist covering IPv4/IPv6, cloud metadata, sensitive paths The PRD aligns well with the retrieved learnings about PRD-based development lifecycle and provides clear acceptance criteria. Minor suggestions for improvement: 1. Consider adding performance benchmarks to success metrics (you mention "minimal performance overhead" but no measurement criteria) 2. The 80%+ test coverage target is good - consider specifying mutation testing for security-critical paths 3. Risk mitigation for "incomplete denylist" suggests "allow updates" - clarify if this means runtime updates or code updates only Based on learnings, this PRD follows the documented approach for larger initiatives requiring multiple PRs. </blockquote></details> <details> <summary>PRDs/20251217-allowlist-resolver/implementation-plan.md (1)</summary><blockquote> `1-1522`: **Excellent TDD-driven implementation plan with clear task breakdown.** This implementation plan demonstrates exemplary test-driven development practices: **Strengths:** - Strict TDD workflow: write failing test → verify failure → implement → verify pass → commit - Clear PR breakdown (7 PRs, ~4-30 files each) targeting ≤50 files per PR - Complete code examples with proper SPDX headers and Javadoc - Good separation of concerns (PR1: interfaces, PR2: denylist, PR3-7: rules/integration/docs) - Integration points clearly identified (AbstractModuleLoader, BindingConstraintLoader, etc.) **Based on learnings:** - Aligns perfectly with "Follow Test-Driven Development (TDD) principles: write tests first before implementing functionality, verify tests fail with current implementation, implement minimal code to pass tests, then refactor while keeping tests green" - Follows PRD-based lifecycle for larger initiatives - Java target version Java 11 with SpotBugs annotations properly shown in examples **Note:** The static analysis warnings about "emphasis used instead of heading" (MD036) are false positives - the bold text like "**Step 1:**" are intentional formatting for TDD step labels, not headings. **Minor suggestion:** Consider splitting this into separate files if it becomes hard to navigate (e.g., `implementation-plan.md` as overview + `pr1-tasks.md`, `pr2-tasks.md`, etc.). However, the current single-file format is acceptable for this level of detail. </blockquote></details> <details> <summary>PRDs/20251208-constraint-builder/implementation-plan.md (1)</summary><blockquote> `1-184`: **Well-scoped implementation plan with clear test migration strategy.** The plan effectively addresses reducing XMLBeans dependencies in test code with: - Focused 2-PR approach keeping changes reviewable - Clear before/after code examples showing the migration path - Thoughtful analysis of which tests to migrate vs. keep XML-based - Integration with existing `IModuleMockFactory` patterns **Based on learnings:** - Follows TDD principles by migrating tests that verify behavior (not XML parsing itself) - Aligns with PRD-based approach for test infrastructure improvements - File count (12-14 total) is well within ≤50 files per PR guideline </blockquote></details> <details> <summary>PRDs/20251208-constraint-builder/PRD.md (1)</summary><blockquote> `1-172`: **Well-structured PRD for test infrastructure improvement.** This PRD effectively addresses technical debt reduction with: - Clear problem statement (eliminate XMLBeans test dependencies) - Focused scope (test support only, not production code) - Concrete requirements with code examples - Realistic success metrics **Strengths:** - Explicit non-goals prevent scope creep - Test migration requirements show before/after examples - Risk analysis acknowledges complexity concerns - Integration with existing `IModuleMockFactory` maintains consistency **Based on learnings:** - Follows PRD-based approach for larger initiatives - Clear delineation that this is test infrastructure (not user-facing constraint documentation) **Note:** The retrieved learning about updating `website/content/specification/syntax/constraints.md` does not apply here, as this is test infrastructure, not user-facing constraint types. </blockquote></details> <details> <summary>databind/.settings/org.eclipse.jdt.core.prefs (1)</summary><blockquote> `115-115`: **`processAnnotations=disabled` does not affect SpotBugs null safety analysis.** Disabling `processAnnotations` disables Eclipse's Annotation Processing Tool (APT), which executes Java 6+ annotation processors (e.g., Lombok, MapStruct). This setting does not affect SpotBugs, which is a separate static analysis tool that operates on compiled bytecode, independent of the compiler's annotation processor configuration. SpotBugs null safety annotations (`@NonNull`, `@Nullable`) are markers for static analysis tools, not code generation processors. This change is safe and does not require reversal. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
david-waltermire
force-pushed
the
feature/183-allowlist-resolver
branch
from
ad43a4a to
0aa8b64
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
PRDs/20251217-allowlist-resolver/PRD.md (1)
83-83: Add language identifiers to code blocks.Markdown code blocks should specify the language for syntax highlighting. Update these blocks:
- Line 83: Add
textidentifier for ASCII diagram- Line 120: Add
textidentifier for class hierarchy- Line 142: Add
textidentifier for integration flow<!-- Before --> \`\`\` ┌─────────────────────... <!-- After --> \`\`\`text ┌─────────────────────...Also applies to: 120-120, 142-142
PRDs/20251217-allowlist-resolver/implementation-plan.md (1)
39-39: Use markdown headings instead of emphasis for structural organization.The document uses bold (
**Step X: ...**) instead of markdown headings (## Step X: ...) for task steps. While this renders correctly, it reduces outline/heading structure clarity for tools and readers. Consider standardizing to markdown headings for consistency.This is a low-priority refactor that can be deferred.
Also applies to: 80-80, 88-88, 172-172, 180-180, 239-239, 253-253, 308-308, 322-322, 382-382, 397-397, 458-458, 466-466, 581-581, 589-589, 601-601, 609-609, 617-617, 635-635, 695-695, 703-703, 853-853, 861-861, 877-877, 961-961, 969-969, 1137-1137, 1145-1145, 1161-1161, 1215-1215, 1221-1221, 1325-1325, 1331-1331, 1347-1347, 1409-1409, 1432-1432, 1440-1440, 1448-1448, 1482-1482, 1489-1489
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PRDs/20251217-allowlist-resolver/PRD.md(1 hunks)PRDs/20251217-allowlist-resolver/implementation-plan.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
PRDs/20251217-allowlist-resolver/PRD.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...k. Issue: [#183 - Add new allowlist only resolver for loading models, instan...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251217-allowlist-resolver/PRD.md
83-83: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
120-120: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
142-142: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
PRDs/20251217-allowlist-resolver/implementation-plan.md
39-39: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
172-172: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
180-180: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
239-239: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
253-253: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
308-308: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
322-322: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
382-382: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
397-397: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
458-458: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
466-466: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
581-581: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
589-589: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
601-601: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
609-609: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
617-617: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
635-635: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
695-695: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
703-703: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
853-853: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
861-861: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
877-877: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
961-961: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
969-969: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1137-1137: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1145-1145: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1161-1161: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1215-1215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1221-1221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1325-1325: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1331-1331: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1347-1347: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1409-1409: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1432-1432: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1440-1440: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1448-1448: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1482-1482: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1489-1489: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (10)
PRDs/20251217-allowlist-resolver/PRD.md (5)
35-76: Design decisions are sound and well-articulated.The defense-in-depth strategy (user allowlist + always-enforced denylist), hierarchical configuration model, and integration points are all clearly thought out. Security-by-default posture (deny all schemes except https) is appropriate for the threat model.
79-171: Architecture design is comprehensive and clearly structured.The component diagram, class hierarchy, and integration flow all demonstrate a well-thought-out layered design. The separation of concerns (SchemePolicy, ResourceRules, BuiltInDenylist) and the defense-in-depth approach are sound.
174-272: API design is well-thought-out and covers realistic use cases.The programmatic builder API is clean and idiomatic; the YAML configuration is hierarchical and flexible. Three loading patterns (file, classpath, environment) support diverse deployment scenarios. The examples demonstrate server-strict, development-permissive, and hierarchical inheritance modes clearly.
276-326: Built-in denylist is comprehensive and addresses common attack vectors.Network patterns cover loopback, private IP ranges, cloud metadata endpoints, and IPv6 variants. Filesystem patterns block system configs (/etc, /proc, /sys), credentials (.ssh, .aws, .gnupg), and Windows equivalents. The pattern set is well-chosen for the threat model.
Note: The implementation plan's use of actual InetAddress resolution (rather than string pattern matching) for network checks is the right approach to avoid bypass attempts.
329-402: Success criteria, testing strategy, and risk mitigation are well-structured.The acceptance criteria span functional, security, and non-functional concerns with clear metrics (80%+ coverage, path traversal blocked, SSRF prevented). Testing strategy covers unit, integration, and security-specific vectors. Risk mitigation appropriately identifies opt-in behavior to minimize breaking changes.
PRDs/20251217-allowlist-resolver/implementation-plan.md (5)
13-24: PR breakdown table is well-structured and logically ordered.The seven-PR progression (core interfaces → denylists → rules → resolver → YAML config → loader integration → docs) follows a sound dependency order. Estimated sizes are realistic for a phased rollout. Each PR has a clear scope and deliverables.
1428-1521: PR3-PR7 outline provides clear roadmap for future implementation PRs.Higher-level scoping is appropriate for a planning document. Task 6.1 example shows the integration pattern clearly (route imports through IUriResolver). Completion checklist provides clear milestone tracking. These PRs will be detailed during their respective implementation cycles.
625-1151: PR2 denylist implementations are thorough and security-conscious.
NetworkDenylistcorrectly leveragesInetAddressmethods (isLoopbackAddress(),isLinkLocalAddress(),isSiteLocalAddress()) to check for blocked network ranges. Handles IPv6 brackets correctly. Tests cover realistic attack vectors (localhost, private ranges, cloud metadata) and legitimate public IPs.
FileSystemDenylistappropriately detects OS at runtime and applies platform-specific rules. Path normalization (lowercase, forward slashes) prevents case and encoding bypasses. Tests are OS-conditional via@EnabledOnOs.
BuiltInDenylistfacade cleanly aggregates both denylists and implementsasRule()for integration.SpotBugs annotations (
@NonNull,@Nullable) are properly configured in the project.
1341-1415: Package-info.java annotations are available but represent a new project pattern.The
spotbugs-annotationsdependency is configured in the project's pom.xml, and the imports in the suggested code are correct. However, existing package-info.java files in the project do not use@DefaultAnnotationForParametersor@DefaultAnnotationForFieldsannotations—they contain only Javadoc. The project currently uses@NonNulland@Nullableannotations in method parameters and fields (e.g., inISchemaGenerator.java), but this would be the first use of DefaultAnnotation directives at the package level. Verify that this pattern aligns with the project's code style guidelines before implementing.
27-595: PR1 design and implementation approach is sound and verified.All dependencies are confirmed present in the codebase:
CollectionUtil.unmodifiableSet()(wrapper aroundCollections.unmodifiableSet()) is available atgov.nist.secauto.metaschema.core.util.CollectionUtiledu.umd.cs.findbugs.annotations.NonNulland related annotations are widely used throughout the projectIUriResolverinterface exists atgov.nist.secauto.metaschema.core.model.IUriResolverand properly supports functional interface patternsLocale.ROOTis standard Java and already used in the codebaseThe TDD workflow (failing test → implementation → passing test → commit) ensures quality. The interfaces are well-designed with proper extension hierarchies and enable composable rule evaluation. Code follows project conventions (SPDX headers, comprehensive javadoc,
@NonNullannotations).
david-waltermire
force-pushed
the
feature/183-allowlist-resolver
branch
from
0aa8b64 to
de629e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
PRDs/20251217-allowlist-resolver/PRD.md (1)
3-3: Fix hyphenated compound adjective (duplicate from previous review).Line 3 still contains "allowlist only resolver" which should be hyphenated as "allowlist-only resolver" to properly form a compound adjective modifying the noun.
-**Issue:** [#183 - Add new allowlist only resolver for loading models, instances, and dynamic model generation](https://github.com/metaschema-framework/metaschema-java/issues/183) +**Issue:** [#183 - Add new allowlist-only resolver for loading models, instances, and dynamic model generation](https://github.com/metaschema-framework/metaschema-java/issues/183)
🧹 Nitpick comments (1)
PRDs/20251217-allowlist-resolver/implementation-plan.md (1)
37-37: Convert emphasis-based section headers to proper markdown headings.Markdownlint reports 41 MD036 violations: lines using bold text (e.g.,
**Step 1: Write the failing test**) should be proper markdown headings. This improves readability and document structure. Convert all step labels and task headers to heading syntax (### Step 1: ...,#### Substep ..., etc.).Example conversion:
-**Step 1: Write the failing test** +### Step 1: Write the failing test -**Step 2: Run test to verify it fails** +### Step 2: Run test to verify it fails -**Step 3: Write implementation** +### Step 3: Write implementationApply this pattern to all ~40 occurrences. Adjust heading level (###, ####) based on hierarchy depth for consistency.
Also applies to: 78-78, 86-86, 170-170, 178-178, 237-237, 251-251, 306-306, 320-320, 380-380, 395-395, 456-456, 464-464, 579-579, 587-587, 599-599, 607-607, 615-615, 633-633, 693-693, 701-701, 851-851, 859-859, 875-875, 959-959, 967-967, 1135-1135, 1143-1143, 1159-1159, 1213-1213, 1219-1219, 1323-1323, 1329-1329, 1345-1345, 1407-1407, 1430-1430, 1438-1438, 1446-1446, 1480-1480, 1487-1487
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PRDs/20251217-allowlist-resolver/PRD.md(1 hunks)PRDs/20251217-allowlist-resolver/implementation-plan.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: For larger initiatives requiring multiple PRs, use structured PRD (Product Requirements Document) approach. PRDs stored in PRDs/<YYYYMMDD>-<name>/ with PRD.md and implementation-plan.md. Target ≤50 files per PR, maximum 100 files per PR.
Applied to files:
PRDs/20251217-allowlist-resolver/PRD.md
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251217-allowlist-resolver/implementation-plan.md
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
78-78: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
86-86: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
170-170: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
178-178: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
237-237: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
251-251: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
306-306: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
320-320: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
380-380: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
395-395: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
456-456: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
464-464: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
579-579: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
587-587: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
599-599: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
607-607: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
615-615: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
633-633: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
693-693: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
701-701: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
851-851: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
859-859: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
875-875: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
959-959: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
967-967: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1135-1135: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1143-1143: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1159-1159: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1213-1213: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1219-1219: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1323-1323: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1329-1329: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1345-1345: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1407-1407: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1430-1430: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1438-1438: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1446-1446: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1480-1480: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1487-1487: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Website
- GitHub Check: Code
🔇 Additional comments (3)
PRDs/20251217-allowlist-resolver/PRD.md (1)
1-439: Comprehensive PRD with strong security posture and clear integration strategy.This PRD effectively documents the allowlist-based URI resolver feature with clear problem statement, defense-in-depth design, and integration across all relevant loading paths. The built-in denylist is appropriately scoped to block high-risk patterns while allowing explicit overrides for legitimate use cases (e.g., localhost testing at lines 325–332). Past questions from david-waltermire regarding YAML/JSON deserializer support (addressed at lines 74–75), scheme organization (shown in lines 239–268), and localhost handling (lines 325–332) appear adequately resolved. The testing strategy is thorough, and success criteria are well-defined. Configuration examples demonstrate both programmatic (lines 178–220) and YAML-based (lines 235–293) approaches clearly.
PRDs/20251217-allowlist-resolver/implementation-plan.md (2)
1-1516: Implementation plan structure is sound and well-aligned with PR objectives.The design follows a sensible incremental approach:
- TDD-driven: Each task includes failing test, implementation, passing test, and commit
- Phased delivery: Split across 7 focused PRs with clear scope
- Security-first: Built-in denylists (network, filesystem) always enforced; hierarchical policies support both global and per-loader overrides
- Comprehensive: Covers interfaces, implementations, integration, and documentation
The code examples follow best practices (immutability, proper null handling, clear JavaDoc). The plan aligns with the PR objectives and PRD design.
1-3: AI directive successfully removed—no further action needed.Verification confirms the AI directive previously flagged on line 3 has been removed. No anthropic-specific, claude-directed, or assistant-instruction comments remain in the document.
david-waltermire
force-pushed
the
feature/183-allowlist-resolver
branch
from
de629e1 to
587637b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
PRDs/20251217-allowlist-resolver/PRD.md (1)
1-615: Standardize markdown heading syntax.Several section headers use bold emphasis (
**Header**) instead of markdown heading syntax (## Header). While this doesn't affect technical accuracy, it breaks markdown semantics and prevents TOC generation. Examples include lines 21, 57, 98, 106, 190, 198, 257, 271, and many others throughout the document.Consider converting section headers to proper markdown heading levels for better document structure and tooling compatibility.
PRDs/20251217-allowlist-resolver/implementation-plan.md (1)
1-2206: Standardize markdown heading syntax (same as PRD.md).Similar to the PRD, many section headers use bold emphasis (
**Header**) instead of markdown heading syntax (## Header). This is prevalent throughout the implementation plan at lines 21, 57, 98, 106, 190, 198, 257, 271, and many others. Convert to proper markdown heading levels for consistency and tooling compatibility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PRDs/20251217-allowlist-resolver/PRD.md(1 hunks)PRDs/20251217-allowlist-resolver/implementation-plan.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: For larger initiatives requiring multiple PRs, use structured PRD (Product Requirements Document) approach. PRDs stored in PRDs/<YYYYMMDD>-<name>/ with PRD.md and implementation-plan.md. Target ≤50 files per PR, maximum 100 files per PR.
Applied to files:
PRDs/20251217-allowlist-resolver/PRD.md
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251217-allowlist-resolver/implementation-plan.md
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
190-190: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
198-198: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
257-257: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
271-271: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
326-326: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
340-340: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
400-400: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
415-415: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
476-476: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
484-484: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
599-599: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
607-607: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
619-619: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
627-627: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
635-635: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
653-653: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
713-713: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
721-721: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
871-871: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
879-879: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
895-895: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
979-979: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
987-987: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1155-1155: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1163-1163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1179-1179: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1233-1233: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1239-1239: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1343-1343: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1349-1349: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1365-1365: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1427-1427: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1450-1450: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1458-1458: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1541-1541: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1556-1556: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1655-1655: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1661-1661: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1923-1923: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1929-1929: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1997-1997: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2107-2107: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2117-2117: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2160-2160: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2167-2167: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2189-2189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2196-2196: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2202-2202: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Website
- GitHub Check: Code
🔇 Additional comments (2)
PRDs/20251217-allowlist-resolver/PRD.md (1)
79-571: Comprehensive and well-structured PRD.The technical design is solid: clear threat model (local file inclusion, SSRF, XXE, scheme injection), defense-in-depth approach, sensible API with builder pattern, and detailed configuration system with merge semantics. Architecture flow, success criteria, and testing strategy are all well-defined.
PRDs/20251217-allowlist-resolver/implementation-plan.md (1)
11-2206: Well-structured implementation plan with disciplined TDD approach.The seven-phase breakdown provides clear logical separation: core interfaces/exceptions, denylists, rules, main resolver, configuration system, YAML/merge support, and integration. Each phase includes specific file locations, failing tests (TDD red phase), implementations, passing tests, and commit messages. Foundational phases (1, 2, 5) are detailed with full code; later phases (3, 4, 6, 7) have clear task outlines. This balance between specificity and flexibility is appropriate for a large feature.
- Define allowlist configuration schema using Metaschema YAML format - Place configuration model in databind-metaschema module for reusability - Add tasks for Maven code generation configuration - Add AllowlistConfigurationLoader using IBoundLoader for multi-format support - Update package structure to gov.nist.secauto.metaschema.databind.metaschema.config
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PRDs/20251217-allowlist-resolver/PRD.md(1 hunks)PRDs/20251217-allowlist-resolver/implementation-plan.md(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: For larger initiatives requiring multiple PRs, use structured PRD (Product Requirements Document) approach. PRDs stored in PRDs/<YYYYMMDD>-<name>/ with PRD.md and implementation-plan.md. Target ≤50 files per PR, maximum 100 files per PR.
Applied to files:
PRDs/20251217-allowlist-resolver/PRD.md
📚 Learning: 2025-12-17T13:27:43.669Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.669Z
Learning: All changes require PR review with CODEOWNERS enforcement
Applied to files:
PRDs/20251217-allowlist-resolver/PRD.md
🪛 markdownlint-cli2 (0.18.1)
PRDs/20251217-allowlist-resolver/implementation-plan.md
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
59-59: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
100-100: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
108-108: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
192-192: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
200-200: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
259-259: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
273-273: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
328-328: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
342-342: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
402-402: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
417-417: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
478-478: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
486-486: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
601-601: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
609-609: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
621-621: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
629-629: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
637-637: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
655-655: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
715-715: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
723-723: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
873-873: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
881-881: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
897-897: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
981-981: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
989-989: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1157-1157: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1165-1165: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1181-1181: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1235-1235: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1241-1241: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1345-1345: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1351-1351: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1367-1367: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1429-1429: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1452-1452: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1460-1460: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1475-1475: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1641-1641: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1655-1655: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1679-1679: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1693-1693: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1777-1777: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1792-1792: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1891-1891: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1897-1897: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2159-2159: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2165-2165: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2235-2235: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2309-2309: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2365-2365: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2381-2381: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2491-2491: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2501-2501: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2544-2544: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2551-2551: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2573-2573: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2580-2580: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
2588-2588: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Code
- GitHub Check: Website
|
|
||
| ### Commit Sequence | ||
|
|
||
| **Phase 1: Core Resolver** |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use markdown heading syntax instead of bold emphasis.
The implementation plan uses bold text (e.g., **Step 1: Write the failing test**, **Goal:**, **Files:**) where proper markdown headings should be used. This violates markdown conventions (MD036) and prevents proper document structure/outline generation.
Convert bold emphasis to heading levels consistently:
**Step 1:** ...→### Step 1: ...**Goal:** ...→### Goal(if needed as subheading) or keep as introductory text**Files:** ...→#### Files(if needed as subheading)
This applies systematically throughout the document—all 47 violations should be corrected for consistency and linting compliance.
Also applies to: 59-59, 100-100, 108-108, 192-192, 200-200, 259-259, 273-273, 328-328, 342-342, 402-402, 417-417, 478-478, 486-486, 601-601, 609-609, 621-621, 629-629, 637-637, 655-655, 715-715, 723-723, 873-873, 881-881, 897-897, 981-981, 989-989, 1157-1157, 1165-1165, 1181-1181, 1235-1235, 1241-1241, 1345-1345, 1351-1351, 1367-1367, 1429-1429, 1452-1452, 1460-1460, 1475-1475, 1641-1641, 1655-1655, 1679-1679, 1693-1693, 1777-1777, 1792-1792, 1891-1891, 1897-1897, 2159-2159, 2165-2165, 2235-2235, 2309-2309, 2365-2365, 2381-2381, 2491-2491, 2501-2501, 2544-2544, 2551-2551
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
PRDs/20251217-allowlist-resolver/implementation-plan.md around lines 21 and the
other listed lines: the document uses bold emphasis (e.g., **Step 1: ...,
**Goal:, **Files:) where markdown headings should be used; replace each bolded
step/goal/files lines with the appropriate heading level (for example convert
"**Step 1: Write the failing test**" to "### Step 1: Write the failing test",
convert "**Goal:** ..." to "### Goal" followed by the goal text, and convert
"**Files:** ..." to "#### Files"); apply this consistently to all 47 flagged
occurrences to conform to MD036 and preserve document outline generation.
| 3. Prevents SSRF attacks to internal services (localhost, cloud metadata endpoints) | ||
| 4. Prevents local file inclusion attacks (directory traversal, sensitive system files) | ||
|
|
||
| ### Security Threats Addressed |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use markdown heading syntax instead of bold emphasis.
The PRD uses bold text (e.g., **Step 1:**, **Goal:**, **Files:**) where proper markdown headings should be used. This violates markdown conventions (MD036) and prevents proper document structure/outline generation.
Convert bold emphasis to heading levels consistently:
**Step 1:** ...→### Step 1: ...**Goal:** ...→### Goal(or appropriate heading level)**Files:** ...→#### Files(or appropriate heading level)
This applies systematically throughout the document—all 38 violations should be corrected for consistency and linting compliance. This matches the same issue in the implementation-plan.md file.
Also applies to: 59-59, 100-100, 108-108, 192-192, 200-200, 259-259, 273-273, 328-328, 342-342, 402-402, 417-417, 478-478, 486-486, 601-601, 609-609, 621-621, 629-629, 637-637, 655-655, 715-715, 723-723, 1367-1367, 1429-1429, 1452-1452, 1460-1460, 1475-1475, 1641-1641, 1655-1655, 1679-1679, 1693-1693, 1777-1777, 1792-1792, 1891-1891, 1897-1897, 2159-2159, 2165-2165
🤖 Prompt for AI Agents
In PRDs/20251217-allowlist-resolver/PRD.md around line 21 (and also at the
following lines: 59, 100, 108, 192, 200, 259, 273, 328, 342, 402, 417, 478, 486,
601, 609, 621, 629, 637, 655, 715, 723, 1367, 1429, 1452, 1460, 1475, 1641,
1655, 1679, 1693, 1777, 1792, 1891, 1897, 2159, 2165) you used bold emphasis for
section labels which violates MD036; replace each instance of bolded labels with
proper Markdown headings using the mapping: change "**Step X:** ..." to "###
Step X: ...", "**Goal:**" to "### Goal", and "**Files:**" to "#### Files" (or
the appropriate heading level consistent with surrounding document structure),
apply this consistently for all 38 occurrences, preserve the text content after
the label, update surrounding blank lines to ensure valid heading separation,
and re-run markdown linting to verify MD036 is resolved.
aj-stein
left a comment
aj-stein
left a comment
There was a problem hiding this comment.
Initial comments, more to follow.
There was a problem hiding this comment.
Since this is a general comment based on previous discussion tonight, I will put at the doc level: are we ok to consider moving to the ".gitignore glob pattern for explicit list or !pattern negations" for an allowlist-first approach, superceding allowlist+denylist method?
Configuration Consolidation NoteWhile working on bug #533 (document loading race condition), I added configurable cache settings to
These follow the existing pattern for When implementing the allowlist resolver configuration model, we should consider consolidating other library-wide settings that users may want to configure:
The hierarchical configuration approach in this PRD (global defaults + per-loader overrides) would work well for these settings too. |
2 participants
Summary
Adds a secure-by-default URI resolver that restricts resource access to explicitly allowed directories, domains, and URI schemes - addressing issue #183.
Goals (from #183)
Design Highlights
Built-in Denylist (cannot be disabled)
Files
PRDs/20251217-allowlist-resolver/PRD.md- Full requirements documentPRDs/20251217-allowlist-resolver/implementation-plan.md- Detailed TDD implementation planImplementation Plan
7 PRs planned:
Resolves #183
Summary by CodeRabbit
Documentation
New Features (planned)
Public API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.