Skip to content

feat(registry): add authentication and ECS deployment support#397

Closed
toadkicker wants to merge 45 commits into
mainfrom
feature/pap-assessment-tool
Closed

feat(registry): add authentication and ECS deployment support#397
toadkicker wants to merge 45 commits into
mainfrom
feature/pap-assessment-tool

Conversation

@toadkicker
Copy link
Copy Markdown
Contributor

Summary

Implemented comprehensive authentication and deployment infrastructure for the PAP registry:

  • Bearer token authentication with constant-time comparison
  • Optional OIDC and API key management (scaffolded)
  • Terraform IaC for ECS deployment in baursoftware-infra
  • Complete end-to-end testing and integration tests

Changes

Core Implementation (Tasks 1-4):

  • ✅ Pluggable auth module with Bearer token validator
  • ✅ Extended Config with OIDC, API keys, auth settings
  • ✅ Auth middleware integrated into Axum router
  • ✅ Optional OIDC + AWS SDK dependencies

UI & Deployment (Tasks 5-7):

  • ✅ Settings page UI for auth status and API key management
  • ✅ Docker Compose updated with auth environment variables
  • ✅ Terraform module for ECS deployment (variables.tf, secrets.tf, main.tf, outputs.tf, README.md)

Testing & Documentation (Tasks 8-11):

  • ✅ Operator & agent integration documentation
  • ✅ Docker image built and pushed to ECR (332745743295.dkr.ecr.us-east-1.amazonaws.com/pap-registry:latest)
  • ✅ End-to-end authentication testing (all passed)
  • ✅ Integration tests for auth middleware (4 tests, all passing)

Test Plan

  • All 210 existing tests pass
  • 4 new integration tests for Bearer token validation
  • End-to-end authentication flow verified locally
  • Docker image builds and pushes to ECR
  • Terraform module validates without errors
  • Documentation complete for operators and agents

Deployment

The registry can now be deployed to baursoftware-infra ECS cluster:

  1. Copy terraform/registry/terraform.tfvars.example to terraform.tfvars
  2. Provide baursoftware-infra infrastructure references (VPC, ECS cluster, ALB)
  3. Run terraform apply

Token is stored securely in AWS Secrets Manager and injected at runtime.

Breaking Changes

None. Federation endpoints remain unauthenticated. Admin API routes require Bearer token when configured.

🤖 Generated with Claude Code

Todd Baur and others added 30 commits May 6, 2026 16:43
Extract and visualize complete PAP codebase architecture as navigable knowledge graph.

Key outputs:
- graph.html: Interactive visualization (vis.js) with 99.3% connectivity
- GRAPH_REPORT.md: Deep architectural analysis with 5-layer spine
- .graphify_extract.json: Full graph data (141 nodes, 190 edges, 22 hyperedges)
- .graphify_communities.json: 10 communities via Louvain clustering
- .graphify_labels.json: Semantic community labels

Analysis reveals:
- Protocol-first design with Mandate/Session/Receipt core
- Two-boundary security model (SD-JWT + OS sandbox)
- Schema.org vocabulary driving UI rendering
- Multi-language bindings (C, C++, C#, Java, Python, TypeScript)
- Federation-ready architecture with Chrysalis registry

99.3% connectivity achieved through strategic bridge discovery across:
- Docs ↔ implementation
- FFI ↔ core protocol
- UI ↔ orchestrator
- Tests ↔ runtime

Extraction cost: 558,894 input / 68,938 output tokens

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Correct graph classification: six-phase handshake IS PAP itself, not a
transport-specific pattern. WebSocket and OHTTP are transport layers that
carry PAP sessions, not define them.

Changes:
- Moved SixPhaseHandshake from transport to protocol layer
- Removed from TransportAbstraction hyperedge
- Added Session -> SixPhaseHandshake (protocol implements handshake)
- Added WebSocketTransport -> Session (transport carries protocol)
- Added OHTTPRelay -> Session (alternative transport)
- Updated report to emphasize transport-agnostic design

The graph now correctly shows PAP as protocol-first with pluggable
transport implementations.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Key features:
- Tab bar for canvas navigation (4 visible + overflow)
- Workflow panel for agent curation + disclosure forms
- Toast notifications for approval requests
- Ghost blocks as aggregated result previews
- Dynamic form generation from AgentAdvertisement schema

Builds on existing CanvasState, IntentPlan, and approval flow infrastructure.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements browser-style tab navigation for canvases with:
- First 4 canvases displayed as visible tabs (MRU sorted)
- Overflow dropdown for remaining canvases
- Active state indication
- Tab close buttons with stop_propagation
- New canvas button
- Relative timestamp formatting in overflow menu

Component follows Leptos patterns from topbar.rs:
- Uses expect_context for CanvasState
- Reactive closures for For/Show
- Clone-before-closure for event handlers
- Memo::new() for derived state

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Active tab with purple bottom border
- Hover states with purple muted background
- Close button hidden by default, visible on hover
- Overflow dropdown with shadow and z-index

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds the CanvasTabBar component to the TopBar view, positioning it
between the header and backdrop elements as a sibling to the topbar.
This enables browser-style tab navigation for canvases directly below
the address bar.

Changes:
- Import CanvasTabBar component in topbar.rs
- Add <CanvasTabBar /> after </header> element
- Tab bar now appears in correct layout position

Part of Papillon Intent Browser UI implementation (Task 3).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Task 4: Workflow Panel State management.

- Added workflow_panel_open: RwSignal<bool> to CanvasState struct
- Initialized to false (closed) in Default impl
- Will be toggled by toast clicks and Ctrl+\ keyboard shortcut

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Task 5 complete. Created WorkflowPanel component with:
- Slide-in drawer from right (backdrop + panel)
- Header with title and close button
- Three sections with placeholders (chat, curation, disclosure)
- Footer with disabled execute button
- Reads workflow_panel_open from CanvasState

Exported in components/mod.rs. Ready for styling (Task 6).

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Task 6: Workflow Panel Styles

- Backdrop: Fixed overlay with fade-in animation
- Panel: 400px right drawer with slide-in animation (250ms)
- Header: Flex layout with close button and purple hover states
- Body: Scrollable content area with section styling
- Sections: Agent info, disclosure list, returns schema, mandate info
- Footer: Approve/reject button styling with purple accent
- Animations: translateX(100%) → translateX(0) for panel slide
- Z-index: backdrop 999, panel 1000

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add WorkflowPanel component to canvas.rs as an overlay.
Panel appears/disappears based on workflow_panel_open signal.
Positioned as final child inside .canvas-page div for proper z-index layering.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- WorkflowChatThread component reads from canvas_state.canvas_messages
- ChatMessage subcomponent with user vs assistant styling
- Empty state when no messages exist
- Replaced placeholder in WorkflowPanel with actual component
- Border colors: purple for user, teal for assistant

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Checkbox selection for agents
- On-device trust badge (teal)
- DID truncation for readability
- Property count display
- Selected state with purple highlight

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Task 10 - dynamic form generation from selected agents' requirements.
- DisclosureForm component computes union of requires_disclosure from selected agents
- DisclosureField subcomponent with field type inference (email, date, url, tel, text)
- Property humanization (strip schema:, capitalize, handle dot notation)
- Form values stored in local HashMap signal
- Approve button shows agent count and disables when no agents selected
- Clean Wing spectrum styling with purple accent

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Integrated AgentCurationList and DisclosureForm into WorkflowPanel with shared state:
- Added placeholder active_plan signal (RwSignal<Option<IntentPlan>>)
- Created shared selected_agents signal that flows from WorkflowPanel to both child components
- Modified AgentCurationList to accept selected_agents as prop instead of managing internal state
- Wrapped workflow body in Show component with "No active plan" fallback
- Added .workflow-no-plan CSS style for empty state display
- All three components (chat, curation, disclosure) now render when plan exists

Task 17 will wire active_plan to real state from canvas/orchestrator.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements Task 12 of the Papillon Intent Browser UI plan.

Created `ApprovalToastStack` component that renders fixed bottom-right toast
notifications for approval requests. Toasts read from `canvas_state.hitl_pending`
and provide two interaction modes:

- **Zero-disclosure agents**: Quick APPROVE button for instant approval
- **Multi-property disclosure**: VIEW DETAILS button opens workflow panel

Key features:
- Slide-in animation from bottom (250ms ease-out)
- Dismiss button (×) clears pending request
- Risk level badges (HIGH/CRITICAL) styled with semantic colors
- Humanized action display (strips "schema:" and "Action" suffix)
- Full unit test coverage for humanize_action logic

Styling follows established patterns from workflow panel (z-index 2000,
uses design system colors and spacing). Backend approval wiring deferred
to Task 18.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add ApprovalToastStack component to canvas page for workflow approval notifications.
- Import ApprovalToastStack component
- Place after WorkflowPanel to overlay canvas content
- Toast will be triggered by hitl_pending signal (backend wiring in Task 18)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Shows schema.org type icon and name
- Agent count indicator
- Type-specific skeletons (Flight, Weather, Article, Generic)
- Block character values (████) for privacy
- Dashed purple border for preview state

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement keyboard shortcuts to enhance canvas navigation UX:
- Ctrl+T: Create new canvas
- Ctrl+W: Close current canvas
- Ctrl+\: Toggle workflow panel
- Ctrl+Tab: Cycle to next canvas

Added cycle_canvas_forward helper that navigates through canvases
in sorted order (by updated_at), wrapping around to the first when
reaching the end. Keyboard handler attached to canvas-page div with
tabindex="0" to enable focus and key event capture.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add active_intent_plan signal to CanvasState and wire it to WorkflowPanel, replacing the local placeholder signal. This enables the workflow panel to display the real intent plan data that will be populated by the canvas_plan_prompt backend command (Task 18).

Changes:
- Add active_intent_plan: RwSignal<Option<IntentPlan>> to CanvasState
- Initialize active_intent_plan: RwSignal::new(None) in Default impl
- Replace local placeholder signal in WorkflowPanel with canvas_state.active_intent_plan
- Remove unused IntentPlan import from workflow_panel.rs

Backend integration (canvas_plan_prompt → active_intent_plan) comes in Task 18.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Create frontend command wrapper for canvas_approve_block Tauri command
and wire disclosure form approval button to call it.

Changes:
- Created frontend/src/commands/approval.rs with ApprovalPayload and SignedChallenge structs
- Created frontend/src/commands/mod.rs to export approval module
- Added commands module to frontend/src/lib.rs
- Updated disclosure_form.rs to call approve_intent_plan on button click
- On approval success: close workflow panel and clear active_intent_plan
- On error: log to console (proper error handling in follow-up)

Note: Block ID and challenge signature are placeholders for now - will be
properly wired in follow-up tasks when identity challenge flow is integrated.

Task 18 complete.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed 3 CSS integration bugs identified by EvidenceQA:

1. Canvas Tab New Button: Changed class from 'canvas-tab-new' to 'new-tab-btn'
   to match CSS definition in main.css (line 9381)

2. Canvas Tab Overflow Dropdown: Updated all overflow-related classes to match CSS:
   - canvas-tab-overflow → overflow-dropdown-container
   - canvas-tab-overflow-btn → overflow-dropdown-toggle
   - canvas-tab-overflow-menu → overflow-dropdown-menu
   - canvas-tab-overflow-item → overflow-dropdown-item

3. Redundant Disabled Button: Removed duplicate "Disclose & Execute" button
   from workflow_panel.rs footer. The functional button with proper wiring
   exists in disclosure_form.rs; the footer button was non-functional.

All component classes now align with CSS selectors, ensuring proper styling.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Replace create_memo with Memo::new (agent_curation_list.rs:48, disclosure_form.rs:18,38,133)
- Replace create_signal with signal (disclosure_form.rs:15)
- Resolves all 5 Leptos deprecation warnings from integration testing

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add .canvas-tabs CSS with display: flex
- Tabs now display horizontally instead of stacking vertically
- Fixes layout bug where tabs appeared in vertical column
8-task implementation plan for multi-agent containers:
- Schema signatures for I/O contracts
- Block containers with visual ports
- Agent selector for multi-agent blocks
- BM25 intent routing (LLM-optional)
- Node-graph wiring with disclosure validation

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- input_types and output_types define agent I/O contract
- can_wire_to() validates connections (subset check)
- matches() ensures agents share same signature in container
- from_agent() extracts signature from AgentInfo

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add three missing tests identified in code review:
- test_matches_identical_signatures: verifies matches() returns true for identical signatures
- test_matches_different_signatures: verifies matches() returns false when input_types or output_types differ
- test_cannot_wire_partial_overlap: verifies can_wire_to() returns false when source outputs only partial required inputs

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- BlockContainer holds N agents with same SchemaSignature
- BlockPosition for 2D canvas layout
- BlockConnection for wired data flow
- WiringError for connection validation
- CanvasBlock.container_id links to container

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- BlockInputPort and BlockOutputPort for visual wiring
- Port circles change color when connected
- Schema type labels stripped of 'schema:' prefix
- CSS positions ports on block edges with hover states

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- AgentSelector shows compatible agents for block
- Toggle selection via checkbox
- Selected state with purple border
- Provider name shown below agent name
- Scrollable list with max 300px height

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Todd Baur and others added 15 commits May 14, 2026 19:19
- BlockContainerView shows multi-agent block
- Input/output ports attached to edges
- Schema type badges with color coding (teal input, gold output)
- Agent count display
- Positioned absolutely for node-graph layout

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Canvas checks for block.container_id field
- BlockContainerView rendering path (fallback to legacy for now)
- Graph mode CSS with grid background
- Backward compatible with existing BlockRenderer

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Uses BM25 IntentIndex to classify prompt
- Derives SchemaSignature from action type
- Filters agents by matching signature
- Returns BlockContainer with compatible agents
- Default position (100, 100)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- connect_blocks validates signature compatibility
- disconnect_blocks removes connections
- Uses SchemaSignature::can_wire_to() for validation
- Prevents cross-canvas connections
- TODO: Database persistence for connections

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… key management

Enhanced Settings page with dedicated sections for:
- Authentication Status: Shows Bearer token and OIDC integration configuration
- API Keys: Placeholder for coming soon self-service key management
- Environment Configuration: Registry setup variables

The UI integrates with the Bearer token middleware and OIDC framework added in prior commits, providing operators with a clear view of their authentication infrastructure.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@gitguardian
Copy link
Copy Markdown

gitguardian Bot commented May 29, 2026

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 29, 2026

Greptile Summary

This PR adds Bearer token authentication to the PAP registry admin routes, introduces a Terraform module for ECS deployment with Secrets Manager integration, and scaffolds OIDC/API-key support. The auth logic itself is sound — constant-time comparison is implemented correctly — but several infrastructure and secrets-hygiene issues need to be addressed before merge.

  • Committed .env with a real token value: apps/registry/.env is tracked in git and contains an actual PAP_REGISTRY_ADMIN_TOKEN value; it should be .gitignored or its token line commented out like .env.example.
  • Terraform admin_token_secret has a weak default: if no tfvars file overrides the variable, Terraform silently provisions Secrets Manager with a known placeholder string, defeating the sensitive = true marking.
  • ECS health check uses plain HTTP while TLS is on by default: the health check command uses http:// rather than https://, meaning every ECS task will fail its health check and the service will never become stable.
  • Real AWS account IDs and resource IDs in the example tfvars: terraform.tfvars.example contains a real account number, VPC ID, and cluster ARN that expose production topology.

Confidence Score: 3/5

Not safe to merge as-is: the committed .env file with a real token and the Terraform default credential are active security concerns, and the HTTP health check will prevent the ECS service from ever reaching a healthy state.

The core auth logic (constant-time comparison, middleware wiring, state integration) is well-implemented. However, the .env file commits a real credential to version control, the Terraform variable for the admin token silently falls back to a known-weak placeholder, the example tfvars file exposes real production AWS identifiers, and the ECS health check targets plain HTTP while the server uses TLS by default.

apps/registry/.env (committed token), terraform/registry/variables.tf (weak default), terraform/registry/main.tf (HTTP health check), terraform/registry/terraform.tfvars.example (real AWS IDs)

Security Review

  • Credential in version control (apps/registry/.env): a real admin Bearer token is committed in a tracked file; any repository reader can use it against deployments that copy the file without modification.
  • Weak Terraform secret default (terraform/registry/variables.tf): admin_token_secret defaults to a hardcoded placeholder string; a terraform apply without an overriding tfvars provisions infrastructure with a publicly-known credential.
  • Infrastructure enumeration (terraform/registry/terraform.tfvars.example): real AWS account ID, VPC ID, and ECS cluster ARN are present in the committed example file, exposing the account topology to anyone with repository access.

Important Files Changed

Filename Overview
apps/registry/.env Committed .env file contains an actual admin token value that should not be in version control; should be gitignored or follow the .env.example commented-out pattern.
apps/registry/src/auth/bearer.rs New bearer token validator using constant-time comparison; correct implementation with good unit test coverage.
apps/registry/src/main.rs Auth middleware wired into admin router; the Bearer prefix check occurs outside the constant-time comparator, which is a minor timing-side-channel concern worth addressing.
apps/registry/src/state.rs Cleanly adds bearer_validator to AppState, initialized from config at startup.
apps/registry/src/config.rs Adds OIDC issuer/audience, AWS region, and API key enable flag to Config struct with correct env-var parsing.
apps/registry/tests/auth_integration_test.rs Integration tests cover happy path, rejection, and disabled-mode cases, but largely duplicate unit tests in bearer.rs rather than testing the middleware end-to-end via HTTP.
terraform/registry/variables.tf Sensitive admin_token_secret variable has a weak hardcoded default that will be silently used if not overridden, enabling an insecure deployment.
terraform/registry/main.tf ECS health check uses plain HTTP while the service defaults to TLS, which will cause the container to be marked unhealthy; ECR pull permissions are also broader than necessary.
terraform/registry/secrets.tf Correctly provisions a Secrets Manager secret and version for the admin token; will be populated from the problematic variable default if not overridden.
terraform/registry/terraform.tfvars.example Contains real AWS account IDs, VPC IDs, and cluster ARNs that expose production infrastructure topology; should use placeholder values.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Middleware as Auth Middleware (main.rs)
    participant Validator as BearerTokenValidator
    participant AdminRouter as Admin Routes

    Client->>Middleware: HTTP request + Authorization header
    Middleware->>Middleware: Extract Bearer prefix
    alt Token configured (admin_token.is_some())
        Middleware->>Validator: validate(Some(token))
        alt Token matches (constant_time_eq)
            Validator-->>Middleware: true
            Middleware->>AdminRouter: next.run(req)
            AdminRouter-->>Client: 200 OK
        else Token mismatch or missing
            Validator-->>Middleware: false
            Middleware-->>Client: 401 Unauthorized
        end
    else No token configured
        Middleware->>AdminRouter: pass-through (no auth layer)
        AdminRouter-->>Client: 200 OK
    end
Loading

Comments Outside Diff (4)

  1. terraform/registry/main.tf, line 1001-1007 (link)

    P1 Health Check Protocol Mismatch (HTTP vs HTTPS)

    The ECS container health check calls http://localhost:${var.port}/federation/identity, but the server enables TLS by default (PAP_REGISTRY_NO_TLS is not set in the task environment). In TLS mode curl -f http://… will receive a TLS handshake response to a plain-HTTP request and return a non-zero exit code, so the container will cycle through startPeriod and eventually be marked unhealthy. The Docker Compose health check correctly uses https://… with -k; the Terraform version should do the same: curl -fk https://localhost:${var.port}/federation/identity || exit 1.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: terraform/registry/main.tf
    Line: 1001-1007
    
    Comment:
    **Health Check Protocol Mismatch (HTTP vs HTTPS)**
    
    The ECS container health check calls `http://localhost:${var.port}/federation/identity`, but the server enables TLS by default (`PAP_REGISTRY_NO_TLS` is not set in the task environment). In TLS mode `curl -f http://…` will receive a TLS handshake response to a plain-HTTP request and return a non-zero exit code, so the container will cycle through `startPeriod` and eventually be marked unhealthy. The Docker Compose health check correctly uses `https://…` with `-k`; the Terraform version should do the same: `curl -fk https://localhost:${var.port}/federation/identity || exit 1`.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code

  2. terraform/registry/main.tf, line 893-924 (link)

    P2 Overly Broad ECR Permissions

    ecr:BatchGetImage and ecr:GetDownloadUrlForLayer are granted with Resource = "*", giving the task execution role access to pull any image in the account. These two actions can be scoped to the specific ECR repository ARN. Only ecr:GetAuthorizationToken actually requires Resource = "*" because it operates at the registry level, not the repository level.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: terraform/registry/main.tf
    Line: 893-924
    
    Comment:
    **Overly Broad ECR Permissions**
    
    `ecr:BatchGetImage` and `ecr:GetDownloadUrlForLayer` are granted with `Resource = "*"`, giving the task execution role access to pull any image in the account. These two actions can be scoped to the specific ECR repository ARN. Only `ecr:GetAuthorizationToken` actually requires `Resource = "*"` because it operates at the registry level, not the repository level.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code

  3. apps/registry/src/main.rs, line 434-460 (link)

    P2 Middleware Auth Check Bypassed on Bearer Prefix Mismatch

    The inline middleware only validates the token if the header value starts with exactly "Bearer " (with a trailing space). The Bearer prefix check happens before the constant-time comparison — meaning the response time for a malformed scheme is detectably different from a correct scheme with a wrong token. Consider factoring this logic into a method on BearerTokenValidator or extractor.rs where it can be tested directly.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: apps/registry/src/main.rs
    Line: 434-460
    
    Comment:
    **Middleware Auth Check Bypassed on `Bearer ` Prefix Mismatch**
    
    The inline middleware only validates the token if the header value starts with exactly `"Bearer "` (with a trailing space). The `Bearer` prefix check happens *before* the constant-time comparison — meaning the response time for a malformed scheme is detectably different from a correct scheme with a wrong token. Consider factoring this logic into a method on `BearerTokenValidator` or `extractor.rs` where it can be tested directly.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    Fix in Claude Code

  4. terraform/registry/variables.tf, line 1185-1190 (link)

    P1 security Insecure Default for Sensitive Auth Variable

    admin_token_secret is marked sensitive = true but still carries a hardcoded placeholder string as its default. Terraform will silently use that placeholder if no tfvars file overrides it — meaning the ECS task can start with authentication enabled and a publicly-known weak credential. Removing the default block entirely forces operators to supply a real value and prevents an accidental insecure deployment.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: terraform/registry/variables.tf
    Line: 1185-1190
    
    Comment:
    **Insecure Default for Sensitive Auth Variable**
    
    `admin_token_secret` is marked `sensitive = true` but still carries a hardcoded placeholder string as its `default`. Terraform will silently use that placeholder if no `tfvars` file overrides it — meaning the ECS task can start with authentication enabled and a publicly-known weak credential. Removing the `default` block entirely forces operators to supply a real value and prevents an accidental insecure deployment.
    
    How can I resolve this? If you propose a fix, please make it concise.

    Fix in Claude Code

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 6 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 6
terraform/registry/main.tf:1001-1007
**Health Check Protocol Mismatch (HTTP vs HTTPS)**

The ECS container health check calls `http://localhost:${var.port}/federation/identity`, but the server enables TLS by default (`PAP_REGISTRY_NO_TLS` is not set in the task environment). In TLS mode `curl -f http://…` will receive a TLS handshake response to a plain-HTTP request and return a non-zero exit code, so the container will cycle through `startPeriod` and eventually be marked unhealthy. The Docker Compose health check correctly uses `https://…` with `-k`; the Terraform version should do the same: `curl -fk https://localhost:${var.port}/federation/identity || exit 1`.

### Issue 2 of 6
terraform/registry/terraform.tfvars.example:19-23
**Real AWS Infrastructure IDs in Example File**

This example file contains real AWS resource identifiers — account ID `332745743295`, VPC ID `vpc-05838cd61af99ae79`, and a concrete ECS cluster ARN. Committing these to a public (or even internal-only) repository exposes the account topology. Prefer generic placeholders like `"vpc-REPLACE_ME"` or `"arn:aws:ecs:us-east-1:ACCOUNT_ID:cluster/CLUSTER_NAME"` in example files.

### Issue 3 of 6
terraform/registry/main.tf:893-924
**Overly Broad ECR Permissions**

`ecr:BatchGetImage` and `ecr:GetDownloadUrlForLayer` are granted with `Resource = "*"`, giving the task execution role access to pull any image in the account. These two actions can be scoped to the specific ECR repository ARN. Only `ecr:GetAuthorizationToken` actually requires `Resource = "*"` because it operates at the registry level, not the repository level.

### Issue 4 of 6
apps/registry/src/main.rs:434-460
**Middleware Auth Check Bypassed on `Bearer ` Prefix Mismatch**

The inline middleware only validates the token if the header value starts with exactly `"Bearer "` (with a trailing space). The `Bearer` prefix check happens *before* the constant-time comparison — meaning the response time for a malformed scheme is detectably different from a correct scheme with a wrong token. Consider factoring this logic into a method on `BearerTokenValidator` or `extractor.rs` where it can be tested directly.

### Issue 5 of 6
apps/registry/.env:30
**Credential Committed to Git**

`apps/registry/.env` is a committed file (not `.gitignore`d) containing an actual token value for `PAP_REGISTRY_ADMIN_TOKEN`. Even labeled as a "test" token, any value committed here becomes a known-credential risk — if this file is copied to a production host without modification, the admin API is immediately accessible by anyone who has read the repository. The `.env.example` file correctly comments the variable out; the `.env` file should either be added to `.gitignore` or match that same commented-out pattern.

### Issue 6 of 6
terraform/registry/variables.tf:1185-1190
**Insecure Default for Sensitive Auth Variable**

`admin_token_secret` is marked `sensitive = true` but still carries a hardcoded placeholder string as its `default`. Terraform will silently use that placeholder if no `tfvars` file overrides it — meaning the ECS task can start with authentication enabled and a publicly-known weak credential. Removing the `default` block entirely forces operators to supply a real value and prevents an accidental insecure deployment.

Reviews (1): Last reviewed commit: "test(registry-auth): add integration tes..." | Re-trigger Greptile

Comment on lines +19 to +23
vpc_id = "vpc-05838cd61af99ae79"
ecs_cluster_name = "ai-agency-mcp-services"
ecs_cluster_arn = "arn:aws:ecs:us-east-1:332745743295:cluster/ai-agency-mcp-services"
alb_target_group_arn = "arn:aws:elasticloadbalancing:us-east-1:332745743295:targetgroup/pap-registry/..."
service_discovery_namespace_id = "ns-xxx"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Real AWS Infrastructure IDs in Example File

This example file contains real AWS resource identifiers — account ID 332745743295, VPC ID vpc-05838cd61af99ae79, and a concrete ECS cluster ARN. Committing these to a public (or even internal-only) repository exposes the account topology. Prefer generic placeholders like "vpc-REPLACE_ME" or "arn:aws:ecs:us-east-1:ACCOUNT_ID:cluster/CLUSTER_NAME" in example files.

Prompt To Fix With AI
This is a comment left during a code review.
Path: terraform/registry/terraform.tfvars.example
Line: 19-23

Comment:
**Real AWS Infrastructure IDs in Example File**

This example file contains real AWS resource identifiers — account ID `332745743295`, VPC ID `vpc-05838cd61af99ae79`, and a concrete ECS cluster ARN. Committing these to a public (or even internal-only) repository exposes the account topology. Prefer generic placeholders like `"vpc-REPLACE_ME"` or `"arn:aws:ecs:us-east-1:ACCOUNT_ID:cluster/CLUSTER_NAME"` in example files.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Comment thread apps/registry/.env
# If true, the server refuses to start if admin_token is not set.
# Default: false. Set to 1 or true.
# PAP_REGISTRY_REQUIRE_AUTH=false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 security Credential Committed to Git

apps/registry/.env is a committed file (not .gitignored) containing an actual token value for PAP_REGISTRY_ADMIN_TOKEN. Even labeled as a "test" token, any value committed here becomes a known-credential risk — if this file is copied to a production host without modification, the admin API is immediately accessible by anyone who has read the repository. The .env.example file correctly comments the variable out; the .env file should either be added to .gitignore or match that same commented-out pattern.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/registry/.env
Line: 30

Comment:
**Credential Committed to Git**

`apps/registry/.env` is a committed file (not `.gitignore`d) containing an actual token value for `PAP_REGISTRY_ADMIN_TOKEN`. Even labeled as a "test" token, any value committed here becomes a known-credential risk — if this file is copied to a production host without modification, the admin API is immediately accessible by anyone who has read the repository. The `.env.example` file correctly comments the variable out; the `.env` file should either be added to `.gitignore` or match that same commented-out pattern.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code

@toadkicker toadkicker force-pushed the feature/pap-assessment-tool branch from c020c05 to baa68fe Compare May 29, 2026 05:39
@toadkicker toadkicker closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant