AndreFerreira5/starranja
1
0
Fork 0
Code Issues 3 Pull requests Projects Releases Packages Wiki Activity Actions

Fix RBAC security concerns and clean up decorator docstrings #168

Merged
Copilot merged 3 commits from copilot/sub-pr-161 into feature/16-rbac-middleware 2025-12-12 01:56:02 +00:00
Conversation 5 Commits 3 Files changed 6 +70 -86
Copilot commented 2025-12-12 01:39:37 +00:00 (Migrated from github.com)
Copy link

Addressed feedback from resolved comments and critical security concerns in the RBAC implementation.

Changes

Code Cleanup

  • src/routes/users.py: Removed prefix="/users" from APIRouter - FastAPI automatically uses filename for route prefix, making this redundant and causing /users/users paths
  • src/authentication/decorators.py: Removed verbose AI-generated comments including:
    • "REFACTORED VERSION - Fixes" header block
    • Multi-example usage snippets in token_required() docstring
    • "Security Considerations" section
    • Redundant example in get_current_user() docstring

Security Enhancements

1. Production Safety for /auth/bootstrap-admin

  • Created separate dev_router for development-only endpoints
  • /auth/bootstrap-admin is now excluded at router registration level when ENVIRONMENT=production
  • Added defense-in-depth with runtime environment check inside endpoint
  • Eliminates attack surface in production deployments

2. Test Suite Reliability

  • Changed admin_token fixture from pytest.skip() to pytest.fail() when ENVIRONMENT=production
  • Prevents silent test skipping that could create false-positive CI results
  • Tests will fail loudly if CI is misconfigured with production environment

3. Error Semantics for Malformed Tokens

  • Changed HTTP 500 to 401 for missing/invalid roles claims in tokens
  • Treats malformed tokens as authentication failures instead of server errors
  • More appropriate error categorization and better security posture

4. Logging Improvements

  • Removed user role arrays from authorization denial logs
  • Reduced verbose logging to minimize sensitive data exposure
  • Maintains audit capability while protecting operational details

5. Breaking Change Documentation

  • Added "API Breaking Changes" section to README.md
  • Documents that /auth/register now requires RBAC authentication (admin/manager roles)
  • Provides migration guidance for clients expecting public registration
  • Explains development-only endpoint behavior

Before/After

Route Prefix

# Before
router = APIRouter(prefix="/users", tags=["Users"])  # Creates /users/users/me

# After
router = APIRouter(tags=["Users"])  # Creates /users/me

Bootstrap Endpoint Security

# Before - Runtime check only
@router.post("/bootstrap-admin")  # Always registered

# After - Router-level exclusion
@dev_router.post("/bootstrap-admin")  # Only registered in non-production
if settings.ENVIRONMENT.lower() != "production":
    app.include_router(auth.dev_router, prefix="/auth", tags=["Auth"])

All linting and formatting checks pass.

šŸ’¬ We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Addressed feedback from resolved comments and critical security concerns in the RBAC implementation. ## Changes ### Code Cleanup - **`src/routes/users.py`**: Removed `prefix="/users"` from APIRouter - FastAPI automatically uses filename for route prefix, making this redundant and causing `/users/users` paths - **`src/authentication/decorators.py`**: Removed verbose AI-generated comments including: - "REFACTORED VERSION - Fixes" header block - Multi-example usage snippets in `token_required()` docstring - "Security Considerations" section - Redundant example in `get_current_user()` docstring ### Security Enhancements **1. Production Safety for /auth/bootstrap-admin** - Created separate `dev_router` for development-only endpoints - `/auth/bootstrap-admin` is now excluded at router registration level when `ENVIRONMENT=production` - Added defense-in-depth with runtime environment check inside endpoint - Eliminates attack surface in production deployments **2. Test Suite Reliability** - Changed `admin_token` fixture from `pytest.skip()` to `pytest.fail()` when `ENVIRONMENT=production` - Prevents silent test skipping that could create false-positive CI results - Tests will fail loudly if CI is misconfigured with production environment **3. Error Semantics for Malformed Tokens** - Changed HTTP 500 to 401 for missing/invalid roles claims in tokens - Treats malformed tokens as authentication failures instead of server errors - More appropriate error categorization and better security posture **4. Logging Improvements** - Removed user role arrays from authorization denial logs - Reduced verbose logging to minimize sensitive data exposure - Maintains audit capability while protecting operational details **5. Breaking Change Documentation** - Added "API Breaking Changes" section to README.md - Documents that `/auth/register` now requires RBAC authentication (admin/manager roles) - Provides migration guidance for clients expecting public registration - Explains development-only endpoint behavior ## Before/After ### Route Prefix ```python # Before router = APIRouter(prefix="/users", tags=["Users"]) # Creates /users/users/me # After router = APIRouter(tags=["Users"]) # Creates /users/me ``` ### Bootstrap Endpoint Security ```python # Before - Runtime check only @router.post("/bootstrap-admin") # Always registered # After - Router-level exclusion @dev_router.post("/bootstrap-admin") # Only registered in non-production if settings.ENVIRONMENT.lower() != "production": app.include_router(auth.dev_router, prefix="/auth", tags=["Auth"]) ``` All linting and formatting checks pass. <!-- START COPILOT CODING AGENT TIPS --> --- šŸ’¬ We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
AndreFerreira5 (Migrated from github.com) reviewed 2025-12-12 01:39:37 +00:00
AndreFerreira5 commented 2025-12-12 01:46:35 +00:00 (Migrated from github.com)
Copy link

@copilot this is also wrong, fix it:

Production safety risk: /auth/bootstrap-admin
    Even though itā€™s blocked when ENVIRONMENT == "production", this endpoint is extremely sensitive.
    If ENVIRONMENT is misconfigured (or missing and default doesnā€™t behave as expected), this is a high-impact security hole.
    Suggestion: ensure:
        Production default is secure (it currently defaults to "production" ā€” good).
        The endpoint is explicitly excluded from router inclusion in prod (stronger than runtime check), or guarded by a separate build/test-only router registration.

Test suite behavior in ā€œproduction CIā€
    admin_token fixture calls pytest.skip() when bootstrap returns 403. Depending on how widely that fixture is used, this may skip a large portion of the auth tests silently.
    If CI accidentally runs with ENVIRONMENT=production, you could get a ā€œgreenā€ suite while skipping RBAC coverage.
    Suggestion: consider failing loudly in CI if ENVIRONMENT=production is detected during tests, unless explicitly intended.

Behavior change: /auth/register is no longer public
    This is intended, but itā€™s a breaking change for any clients/scripts expecting to self-register.
    Ensure API docs / README / frontend expectations are updated accordingly (not shown in this PR), or add a separate public ā€œself-registerā€ flow if desired.

Error semantics for missing/invalid roles claim
    The decorator returns 500 if the token is missing roles or roles is not a list.
    Thatā€™s defensible as ā€œtoken service misconfiguration,ā€ but it can also turn malformed tokens into 500s, which may not be ideal externally.
    Consider whether youā€™d rather treat malformed tokens as 401 (invalid token) instead of 500.

Logging may leak operational detail
    There are multiple logs about auth failures and role sets. Itā€™s useful, but be careful:
        Logging full roles might be acceptable; logging user_id + roles on denied access might be okay, but confirm your privacy/audit policy.
        Avoid logging raw tokens (I didnā€™t see raw token logging, which is good).
@copilot this is also wrong, fix it: Production safety risk: /auth/bootstrap-admin Even though itā€™s blocked when ENVIRONMENT == "production", this endpoint is extremely sensitive. If ENVIRONMENT is misconfigured (or missing and default doesnā€™t behave as expected), this is a high-impact security hole. Suggestion: ensure: Production default is secure (it currently defaults to "production" ā€” good). The endpoint is explicitly excluded from router inclusion in prod (stronger than runtime check), or guarded by a separate build/test-only router registration. Test suite behavior in ā€œproduction CIā€ admin_token fixture calls pytest.skip() when bootstrap returns 403. Depending on how widely that fixture is used, this may skip a large portion of the auth tests silently. If CI accidentally runs with ENVIRONMENT=production, you could get a ā€œgreenā€ suite while skipping RBAC coverage. Suggestion: consider failing loudly in CI if ENVIRONMENT=production is detected during tests, unless explicitly intended. Behavior change: /auth/register is no longer public This is intended, but itā€™s a breaking change for any clients/scripts expecting to self-register. Ensure API docs / README / frontend expectations are updated accordingly (not shown in this PR), or add a separate public ā€œself-registerā€ flow if desired. Error semantics for missing/invalid roles claim The decorator returns 500 if the token is missing roles or roles is not a list. Thatā€™s defensible as ā€œtoken service misconfiguration,ā€ but it can also turn malformed tokens into 500s, which may not be ideal externally. Consider whether youā€™d rather treat malformed tokens as 401 (invalid token) instead of 500. Logging may leak operational detail There are multiple logs about auth failures and role sets. Itā€™s useful, but be careful: Logging full roles might be acceptable; logging user_id + roles on denied access might be okay, but confirm your privacy/audit policy. Avoid logging raw tokens (I didnā€™t see raw token logging, which is good).
Copilot commented 2025-12-12 01:47:50 +00:00 (Migrated from github.com)
Copy link

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • astral.sh
    • Triggering command: /usr/bin/curl curl -LsSf REDACTED (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `astral.sh` > - Triggering command: `/usr/bin/curl curl -LsSf REDACTED` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/AndreFerreira5/starranja/settings/copilot/coding_agent) (admins only) > > </details>
Copilot commented 2025-12-12 01:53:42 +00:00 (Migrated from github.com)
Copy link

@copilot this is also wrong, fix it:

Production safety risk: /auth/bootstrap-admin

...

All security concerns addressed in commit fe1e588:

1. Bootstrap-admin protection (Resolved)

  • Moved to separate dev_router excluded at router registration level
  • Only included when ENVIRONMENT != "production"
  • Defense-in-depth: endpoint still checks environment internally

2. Test suite production safety (Resolved)

  • admin_token fixture now uses pytest.fail() instead of pytest.skip()
  • Tests will fail loudly if ENVIRONMENT=production, preventing false-positive CI

3. Breaking change documentation (Resolved)

  • Added "API Breaking Changes" section to README.md
  • Documents RBAC protection on /auth/register
  • Migration guidance provided

4. Malformed token error semantics (Resolved)

  • Missing/invalid roles claim now returns 401 instead of 500
  • Treats as authentication failure (invalid token) not server error

5. Sensitive data in logs (Resolved)

  • Removed user role arrays from authorization denial logs
  • Reduced verbose logging to only log user_id
  • Changed from logger.error to logger.warning for malformed tokens

All linting/formatting checks pass. Ready for review.

> @copilot this is also wrong, fix it: > > Production safety risk: /auth/bootstrap-admin ... All security concerns addressed in commit fe1e588: **1. Bootstrap-admin protection (Resolved)** - Moved to separate `dev_router` excluded at router registration level - Only included when `ENVIRONMENT != "production"` - Defense-in-depth: endpoint still checks environment internally **2. Test suite production safety (Resolved)** - `admin_token` fixture now uses `pytest.fail()` instead of `pytest.skip()` - Tests will fail loudly if `ENVIRONMENT=production`, preventing false-positive CI **3. Breaking change documentation (Resolved)** - Added "API Breaking Changes" section to README.md - Documents RBAC protection on `/auth/register` - Migration guidance provided **4. Malformed token error semantics (Resolved)** - Missing/invalid roles claim now returns 401 instead of 500 - Treats as authentication failure (invalid token) not server error **5. Sensitive data in logs (Resolved)** - Removed user role arrays from authorization denial logs - Reduced verbose logging to only log user_id - Changed from `logger.error` to `logger.warning` for malformed tokens All linting/formatting checks pass. Ready for review.
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-12-12 02:01:01 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

Pull request overview

This PR addresses critical RBAC security concerns and cleans up decorator docstrings. The main focus is on preventing the /auth/bootstrap-admin endpoint from being available in production through router-level exclusion, improving test reliability by failing loudly on environment misconfiguration, and treating malformed tokens as authentication failures (401) rather than server errors (500).

Key Changes:

  • Separated development-only endpoints into a dev_router that is conditionally registered only in non-production environments
  • Changed test fixtures to use pytest.fail() instead of pytest.skip() to prevent false-positive CI results from silent test skipping
  • Updated error handling to return HTTP 401 for malformed token claims instead of HTTP 500, improving security semantics

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/conftest.py Updated admin_token fixture to fail loudly when ENVIRONMENT=production instead of silently skipping tests, preventing false-positive CI results
src/routes/users.py Removed redundant prefix="/users" from APIRouter (prefix is set in main.py during router registration)
src/routes/auth.py Created separate dev_router for bootstrap-admin endpoint and reduced logging verbosity by removing user roles from audit logs
src/main.py Added conditional registration of dev_router based on ENVIRONMENT setting, excluding development endpoints from production deployments
src/authentication/decorators.py Changed HTTP status codes from 500ā†’401 for malformed tokens, updated log levels from errorā†’warning for client-side token issues, and removed verbose AI-generated documentation
README.md Added "API Breaking Changes" section documenting that /auth/register now requires RBAC authentication and bootstrap-admin is development-only

šŸ’” Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Pull request overview This PR addresses critical RBAC security concerns and cleans up decorator docstrings. The main focus is on preventing the `/auth/bootstrap-admin` endpoint from being available in production through router-level exclusion, improving test reliability by failing loudly on environment misconfiguration, and treating malformed tokens as authentication failures (401) rather than server errors (500). **Key Changes:** - Separated development-only endpoints into a `dev_router` that is conditionally registered only in non-production environments - Changed test fixtures to use `pytest.fail()` instead of `pytest.skip()` to prevent false-positive CI results from silent test skipping - Updated error handling to return HTTP 401 for malformed token claims instead of HTTP 500, improving security semantics ### Reviewed changes Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments. <details> <summary>Show a summary per file</summary> | File | Description | | ---- | ----------- | | `tests/conftest.py` | Updated `admin_token` fixture to fail loudly when ENVIRONMENT=production instead of silently skipping tests, preventing false-positive CI results | | `src/routes/users.py` | Removed redundant `prefix="/users"` from APIRouter (prefix is set in main.py during router registration) | | `src/routes/auth.py` | Created separate `dev_router` for bootstrap-admin endpoint and reduced logging verbosity by removing user roles from audit logs | | `src/main.py` | Added conditional registration of dev_router based on ENVIRONMENT setting, excluding development endpoints from production deployments | | `src/authentication/decorators.py` | Changed HTTP status codes from 500ā†’401 for malformed tokens, updated log levels from errorā†’warning for client-side token issues, and removed verbose AI-generated documentation | | `README.md` | Added "API Breaking Changes" section documenting that /auth/register now requires RBAC authentication and bootstrap-admin is development-only | </details> --- šŸ’” <a href="/AndreFerreira5/starranja/new/main/.github/instructions?filename=*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
README.md
@ -26,0 +32,4 @@
- **Previous behavior**: Public endpoint - anyone could register a new user
- **New behavior**: Protected endpoint - requires authentication with admin, gerente, or mecanico_gerente roles
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-12-12 02:01:00 +00:00
Copy link

The role name "gerente" appears in the README documentation but should be verified for consistency. The PR description mentions "admin/manager roles" but the actual role names in the code are "admin", "gerente", and "mecanico_gerente". Using Spanish "gerente" while the rest of the documentation is in English could be confusing for API consumers. Consider documenting all valid role names explicitly or using consistent language.

- **New behavior**: Protected endpoint - requires authentication with one of the following roles: `admin`, `gerente`, or `mecanico_gerente`

**Valid role names:**

| Role name           | Meaning (English)   |
|---------------------|--------------------|
| `admin`             | Administrator      |
| `gerente`           | Manager            |
| `mecanico_gerente`  | Mechanic Manager   |
The role name "gerente" appears in the README documentation but should be verified for consistency. The PR description mentions "admin/manager roles" but the actual role names in the code are "admin", "gerente", and "mecanico_gerente". Using Spanish "gerente" while the rest of the documentation is in English could be confusing for API consumers. Consider documenting all valid role names explicitly or using consistent language. ```suggestion - **New behavior**: Protected endpoint - requires authentication with one of the following roles: `admin`, `gerente`, or `mecanico_gerente` **Valid role names:** | Role name | Meaning (English) | |---------------------|--------------------| | `admin` | Administrator | | `gerente` | Manager | | `mecanico_gerente` | Mechanic Manager | ```
src/routes/auth.py
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-12-12 02:01:01 +00:00
Copy link

The use of getattr(settings, "ENVIRONMENT", "production") is unnecessary because the Settings class in src/config.py already defines ENVIRONMENT: str = "production" as a field with a default value. The attribute is guaranteed to exist, so this can be simplified to settings.ENVIRONMENT.lower() for consistency with how it's accessed in src/main.py line 66.

    if settings.ENVIRONMENT.lower() == "production":
The use of `getattr(settings, "ENVIRONMENT", "production")` is unnecessary because the `Settings` class in `src/config.py` already defines `ENVIRONMENT: str = "production"` as a field with a default value. The attribute is guaranteed to exist, so this can be simplified to `settings.ENVIRONMENT.lower()` for consistency with how it's accessed in `src/main.py` line 66. ```suggestion if settings.ENVIRONMENT.lower() == "production": ```
src/routes/auth.py
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-12-12 02:01:01 +00:00
Copy link

The logging message at line 234 was changed to remove the user's roles array, which is good for reducing sensitive data exposure. However, the log message "User registration requested by user_id: %s" could be more informative by including the target username being registered. Consider: "User registration requested by user_id: %s for username: %s" to maintain audit capability while avoiding logging the requester's roles.

            "User registration requested by user_id: %s for username: %s",
            current_user.get("user_id"),
            request.username,
The logging message at line 234 was changed to remove the user's roles array, which is good for reducing sensitive data exposure. However, the log message "User registration requested by user_id: %s" could be more informative by including the target username being registered. Consider: "User registration requested by user_id: %s for username: %s" to maintain audit capability while avoiding logging the requester's roles. ```suggestion "User registration requested by user_id: %s for username: %s", current_user.get("user_id"), request.username, ```
tests/conftest.py
@ -263,0 +263,4 @@
# Check if we're in production mode - fail loudly if so
env_value = os.getenv("ENVIRONMENT", "").lower()
if env_value == "production":
pytest.fail(
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-12-12 02:01:00 +00:00
Copy link

The environment check uses os.getenv("ENVIRONMENT", "").lower() which defaults to empty string if the environment variable is not set. An empty string would not equal "production", so the test would proceed. However, in src/routes/auth.py line 148, the code uses getattr(settings, "ENVIRONMENT", "production") which defaults to "production" if unset. This inconsistency means the test fixture might allow tests to run when ENVIRONMENT is unset, but the bootstrap endpoint would block them. Consider using the same default value ("production") in both places for defense-in-depth, or documenting why they differ.

    env_value = os.getenv("ENVIRONMENT", "production").lower()
The environment check uses `os.getenv("ENVIRONMENT", "").lower()` which defaults to empty string if the environment variable is not set. An empty string would not equal "production", so the test would proceed. However, in `src/routes/auth.py` line 148, the code uses `getattr(settings, "ENVIRONMENT", "production")` which defaults to "production" if unset. This inconsistency means the test fixture might allow tests to run when ENVIRONMENT is unset, but the bootstrap endpoint would block them. Consider using the same default value ("production") in both places for defense-in-depth, or documenting why they differ. ```suggestion env_value = os.getenv("ENVIRONMENT", "production").lower() ```
Sign in to join this conversation.
Reviewers
No reviewers
AndreFerreira5
Labels
Clear labels   
CI/CD

   
MongoDB

   
PostgreSQL

   
api

   
authentication

   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
example issue

issue that aims to show how to create issues

  
fix

   
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
high-level requirement

   
infrastructure

   
invalid

This doesn't seem right

  
low-level requirement

   
medium-level requirement

   
question

Further information is requested

  
security

   
test

   
wontfix

This will not be worked on

No labels
CI/CD
MongoDB
PostgreSQL
api
authentication
bug
documentation
duplicate
enhancement
example issue
fix
good first issue
help wanted
high-level requirement
infrastructure
invalid
low-level requirement
medium-level requirement
question
security
test
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
AndreFerreira5/starranja!168
No description provided.