feat: add basic web UI authentication with environment variables#519
Closed
rowanchen-com wants to merge 1 commit intoagentscope-ai:mainfrom
Closed
feat: add basic web UI authentication with environment variables#519rowanchen-com wants to merge 1 commit intoagentscope-ai:mainfrom
rowanchen-com wants to merge 1 commit intoagentscope-ai:mainfrom
Conversation
Contributor
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
Review Summary by QodoAdd web UI authentication with JWT tokens and environment-based credentials
WalkthroughsDescription• Implement authentication system with JWT tokens and salted password hashing • Add login page UI with form validation and error handling • Protect API endpoints with Bearer token middleware authentication • Initialize credentials from ADMIN_USERNAME and ADMIN_PASSWORD environment variables • Add logout functionality and auth status checking to sidebar Diagramflowchart LR
A["Environment Variables<br/>ADMIN_USERNAME<br/>ADMIN_PASSWORD"] -->|init_auth_from_env| B["auth.json<br/>Credentials Storage"]
C["Login Form"] -->|POST /api/auth/login| D["authenticate()"]
D -->|verify password| B
D -->|create JWT token| E["Token"]
E -->|localStorage| F["Frontend"]
F -->|Bearer Token| G["AuthMiddleware"]
G -->|verify_token| H["Protected API"]
I["Sidebar"] -->|clearAuthToken| J["Logout"]
File Changes1. src/copaw/app/auth.py
|
Code Review by Qodo
1.
|
📝 WalkthroughWalkthroughAdds end-to-end authentication: backend auth module, middleware and /auth endpoints; frontend login page, token storage and AuthGuard-protected routes; global 401 handling, logout UI, translations, and deployment defaults for admin credentials. Changes
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant Frontend as Frontend (React)
participant AuthGuard as AuthGuard
participant API as Backend API
participant Auth as Auth Module
participant Storage as localStorage
User->>Frontend: Navigate to protected route
Frontend->>AuthGuard: Mount / check
AuthGuard->>API: GET /auth/status
API->>Auth: is_auth_enabled()
Auth-->>API: enabled? true/false
API-->>AuthGuard: AuthStatusResponse
alt Auth Disabled
AuthGuard->>Frontend: Allow access
else Auth Enabled
AuthGuard->>Storage: Read token
alt No token
AuthGuard->>User: Redirect to /login?redirect=...
else Token present
AuthGuard->>API: Validate (e.g., GET /api/version)
API->>Auth: verify_token()
alt Token valid
Auth-->>API: username
API-->>AuthGuard: OK
AuthGuard->>Frontend: Allow access
else Invalid/expired
AuthGuard->>Storage: Clear token
AuthGuard->>User: Redirect to /login
end
end
end
sequenceDiagram
participant User as User/Browser
participant LoginPage as LoginPage
participant Frontend as Frontend API Client
participant API as Backend API
participant Auth as Auth Module
participant Storage as localStorage
User->>LoginPage: Submit credentials
LoginPage->>Frontend: authApi.login(username,password)
Frontend->>API: POST /auth/login
API->>Auth: authenticate(username,password)
alt Auth disabled
Auth-->>API: indicates disabled
API-->>Frontend: LoginResponse (message, no token)
Frontend-->>LoginPage: Show auth-not-enabled message
else Auth enabled & credentials valid
Auth-->>API: token
API-->>Frontend: LoginResponse(token, username)
Frontend->>Storage: setAuthToken(token)
Frontend->>User: Redirect to saved redirect or /chat
else Invalid credentials
API-->>Frontend: HTTP 401
Frontend-->>LoginPage: Show login failed
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/copaw/app/routers/auth.py (1)
35-37: Move import to module level.The lazy import of
HTTPExceptioninside the function is unconventional. Moving it to the top improves readability and avoids repeated import overhead:Suggested fix
from fastapi import APIRouter +from fastapi import HTTPException from pydantic import BaseModel ... if token is None: - from fastapi import HTTPException - raise HTTPException(status_code=401, detail="Invalid credentials")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/routers/auth.py` around lines 35 - 37, Move the inline import of HTTPException out of the function and into the module-level imports: add "from fastapi import HTTPException" at the top of the file and remove the local "from fastapi import HTTPException" inside the function that raises HTTPException (the block that currently does raise HTTPException(status_code=401, detail="Invalid credentials")). This avoids repeated imports and matches project style while keeping the raise statement unchanged.src/copaw/app/auth.py (1)
151-158: Defaulting to a known password when env var is empty may be unexpected.When
ADMIN_PASSWORDis empty or unset, the code defaults to"copaw". This means authentication is always enabled with a weak default password, which could catch deployers off guard.Consider treating an empty password as "auth disabled" to align with the PR description's note about defaulting to open-access behavior:
Suggested alternative behavior
def init_auth_from_env() -> None: username = os.environ.get("ADMIN_USERNAME", "admin").strip() - password = os.environ.get("ADMIN_PASSWORD", "copaw").strip() + password = os.environ.get("ADMIN_PASSWORD", "").strip() - if not username: - username = "admin" + if not password: + logger.info("ADMIN_PASSWORD not set - authentication disabled") + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/auth.py` around lines 151 - 158, The init_auth_from_env function currently defaults ADMIN_PASSWORD to "copaw" when the env var is unset/empty, enabling auth with a weak password; change the logic so that an empty or missing ADMIN_PASSWORD is treated as "auth disabled" (do not create/overwrite credentials in auth.json), and only enable/update credentials when ADMIN_PASSWORD is non-empty; reference the init_auth_from_env function, the ADMIN_PASSWORD and ADMIN_USERNAME environment lookups, and the code path that writes/updates auth.json to gate that update on a non-empty password value.console/src/App.tsx (1)
58-58: Preserve full route context in login redirect.Only
window.location.pathnameis encoded right now, so query/hash context is lost after login.↩️ Suggested improvement
- if (status === "auth-required") - return <Navigate to={`/login?redirect=${encodeURIComponent(window.location.pathname)}`} replace />; + if (status === "auth-required") { + const returnTo = `${window.location.pathname}${window.location.search}${window.location.hash}`; + return <Navigate to={`/login?redirect=${encodeURIComponent(returnTo)}`} replace />; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/App.tsx` at line 58, The redirect currently only preserves window.location.pathname in the Navigate return inside App.tsx, losing query and hash parts; update the redirect to include the full route context by encoding and passing pathname + search + hash (i.e., window.location.pathname combined with window.location.search and window.location.hash) to the redirect query parameter so the post-login redirect restores query string and fragment. Locate the Navigate return expression and replace the redirect value to use the combined, encodeURIComponent-encoded full location string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@console/src/App.tsx`:
- Around line 24-54: The inner token verification currently uses a hardcoded
fetch("/api/version") and does not handle fetch rejections, which can leave
status stuck; update the auth check inside the useEffect (where
authApi.getStatus, getApiToken and clearAuthToken are used) to call the
configured API helper instead of a hardcoded path (e.g., use the existing API
URL helper or an authApi.getVersion/ping method) and wrap the token verification
request in a promise chain with a .catch that handles network errors by setting
setStatus("ok"); keep the existing logic that on an ok response sets
setStatus("ok") and on a non-ok response calls clearAuthToken() then
setStatus("auth-required").
In `@console/src/layouts/Sidebar.tsx`:
- Around line 63-65: The dynamic import of "../api/modules/auth" used to call
authApi.getStatus() can reject and currently has no top-level catch; update the
async flow around import("../api/modules/auth") so you handle module-load
failures (add a .catch or use try/catch in an async IIFE) and ensure the UI
state is deterministic by setting setAuthEnabled(false) or leaving previous
state on import failure; also retain the existing .catch() on
authApi.getStatus() but move error handling to log the error or
setAuthEnabled(false) so Sidebar's auth state doesn't become inconsistent —
focus changes around the dynamic import and the authApi.getStatus() call sites
(the import("../api/modules/auth") line, authApi.getStatus(), and
setAuthEnabled).
In `@console/src/pages/Login/index.tsx`:
- Around line 22-29: The redirect query param is used directly; validate and
sanitize it before calling navigate. Replace using redirect from
searchParams.get(...) with a sanitized value (e.g., derive sanitizedRedirect
from redirect) that enforces an internal-only path: must start with a single
slash ("/"), must not start with "//", must not contain "://" or full hostnames,
and optionally match an allowlist (e.g., default "/chat"). Use sanitizedRedirect
in navigate(...) (inside the Login component where setAuthToken(res.token) and
navigate are called) so external/absolute URLs are rejected and navigation
always stays internal.
In `@deploy/Dockerfile`:
- Around line 19-21: Remove the baked-in ADMIN_PASSWORD from the Dockerfile to
avoid exposing credentials: delete or omit the ENV ADMIN_PASSWORD=copaw line and
keep only ENV ADMIN_USERNAME=admin (if desired); then update the application
startup or entrypoint logic (the component that reads ADMIN_PASSWORD at runtime)
to treat an unset ADMIN_PASSWORD as "no password / open-access" or to fail-fast
with a clear error if you prefer requiring it at runtime, ensuring the runtime
check references ADMIN_PASSWORD when deciding auth mode.
In `@docker-compose.yml`:
- Around line 9-12: Replace the hardcoded environment values in
docker-compose.yml with environment variable interpolation using defaults (e.g.,
change ADMIN_USERNAME=admin to ADMIN_USERNAME=${ADMIN_USERNAME:-admin},
ADMIN_PASSWORD=copaw to ADMIN_PASSWORD=${ADMIN_PASSWORD:-copaw}, and
COPAW_PORT=8088 to COPAW_PORT=${COPAW_PORT:-8088}) so credentials can be
overridden via a .env file; add a .env.example documenting ADMIN_USERNAME,
ADMIN_PASSWORD, and COPAW_PORT and ensure .env is listed in .gitignore to avoid
committing secrets.
In `@src/copaw/app/_app.py`:
- Around line 61-63: The startup currently calls init_auth_from_env()
unconditionally which can create default or blank admin credentials; update the
bootstrap to only call init_auth_from_env() when the expected admin credential
environment variables are present and non-empty (e.g., check for the specific
ADMIN_USER/ADMIN_PASSWORD env vars your auth expects) and skip initialization
otherwise to preserve true open-access fallback; locate the call to
init_auth_from_env() in the app startup (function/module where it’s invoked) and
wrap it with a guard that validates the required env values before invoking
init_auth_from_env().
In `@src/copaw/app/auth.py`:
- Around line 52-63: Replace the single-iteration SHA256 in _hash_password and
verify_password with PBKDF2-HMAC-SHA256 using a high iteration count (e.g.,
600000); generate a random salt as bytes (secrets.token_bytes), run
hashlib.pbkdf2_hmac('sha256', password_bytes, salt_bytes, iterations), and
return hex-encoded derived key and hex-encoded salt from _hash_password, then
have verify_password recompute the PBKDF2 result from the provided salt and
compare with hmac.compare_digest; update function signatures/returns for
_hash_password and verify_password accordingly and note that existing auth.json
entries must be re-initialized after this change.
In `@src/copaw/app/routers/auth.py`:
- Around line 27-39: The login handler returns a plain dict with a message when
auth is disabled which doesn't match the Pydantic LoginResponse model; either
update the LoginResponse model to include an optional message: str | None (or
Optional[str]) so the model matches the frontend, or change the early return in
login to return a LoginResponse instance (e.g., LoginResponse(token="",
username="", message="Auth not enabled")) so the response type is consistent;
locate the LoginResponse model definition and the login function to apply one of
these fixes.
---
Nitpick comments:
In `@console/src/App.tsx`:
- Line 58: The redirect currently only preserves window.location.pathname in the
Navigate return inside App.tsx, losing query and hash parts; update the redirect
to include the full route context by encoding and passing pathname + search +
hash (i.e., window.location.pathname combined with window.location.search and
window.location.hash) to the redirect query parameter so the post-login redirect
restores query string and fragment. Locate the Navigate return expression and
replace the redirect value to use the combined, encodeURIComponent-encoded full
location string.
In `@src/copaw/app/auth.py`:
- Around line 151-158: The init_auth_from_env function currently defaults
ADMIN_PASSWORD to "copaw" when the env var is unset/empty, enabling auth with a
weak password; change the logic so that an empty or missing ADMIN_PASSWORD is
treated as "auth disabled" (do not create/overwrite credentials in auth.json),
and only enable/update credentials when ADMIN_PASSWORD is non-empty; reference
the init_auth_from_env function, the ADMIN_PASSWORD and ADMIN_USERNAME
environment lookups, and the code path that writes/updates auth.json to gate
that update on a non-empty password value.
In `@src/copaw/app/routers/auth.py`:
- Around line 35-37: Move the inline import of HTTPException out of the function
and into the module-level imports: add "from fastapi import HTTPException" at
the top of the file and remove the local "from fastapi import HTTPException"
inside the function that raises HTTPException (the block that currently does
raise HTTPException(status_code=401, detail="Invalid credentials")). This avoids
repeated imports and matches project style while keeping the raise statement
unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
console/src/App.tsxconsole/src/api/config.tsconsole/src/api/modules/auth.tsconsole/src/api/request.tsconsole/src/layouts/Sidebar.tsxconsole/src/locales/en.jsonconsole/src/locales/zh.jsonconsole/src/pages/Login/index.tsxdeploy/Dockerfiledocker-compose.ymlsrc/copaw/app/_app.pysrc/copaw/app/auth.pysrc/copaw/app/routers/__init__.pysrc/copaw/app/routers/auth.py
console/src/layouts/Sidebar.tsx
Outdated
Comment on lines
+63
to
+65
| import("../api/modules/auth").then(({ authApi }) => { | ||
| authApi.getStatus().then((res) => setAuthEnabled(res.enabled)).catch(() => {}); | ||
| }); |
There was a problem hiding this comment.
Handle dynamic import failure in auth status check.
import("../api/modules/auth") has no top-level .catch(). A failed module load can cause an unhandled rejection and inconsistent auth UI state.
✅ Suggested fix
- import("../api/modules/auth").then(({ authApi }) => {
- authApi.getStatus().then((res) => setAuthEnabled(res.enabled)).catch(() => {});
- });
+ import("../api/modules/auth")
+ .then(({ authApi }) => authApi.getStatus())
+ .then((res) => setAuthEnabled(res.enabled))
+ .catch(() => {
+ setAuthEnabled(false);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import("../api/modules/auth").then(({ authApi }) => { | |
| authApi.getStatus().then((res) => setAuthEnabled(res.enabled)).catch(() => {}); | |
| }); | |
| import("../api/modules/auth") | |
| .then(({ authApi }) => authApi.getStatus()) | |
| .then((res) => setAuthEnabled(res.enabled)) | |
| .catch(() => { | |
| setAuthEnabled(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@console/src/layouts/Sidebar.tsx` around lines 63 - 65, The dynamic import of
"../api/modules/auth" used to call authApi.getStatus() can reject and currently
has no top-level catch; update the async flow around
import("../api/modules/auth") so you handle module-load failures (add a .catch
or use try/catch in an async IIFE) and ensure the UI state is deterministic by
setting setAuthEnabled(false) or leaving previous state on import failure; also
retain the existing .catch() on authApi.getStatus() but move error handling to
log the error or setAuthEnabled(false) so Sidebar's auth state doesn't become
inconsistent — focus changes around the dynamic import and the
authApi.getStatus() call sites (the import("../api/modules/auth") line,
authApi.getStatus(), and setAuthEnabled).
deploy/Dockerfile
Outdated
Comment on lines
+19
to
+21
| # Default auth credentials (override at runtime with -e ADMIN_USERNAME=... -e ADMIN_PASSWORD=...) | ||
| ENV ADMIN_USERNAME=admin | ||
| ENV ADMIN_PASSWORD=copaw |
There was a problem hiding this comment.
Default password in Dockerfile is visible in image layers.
The ENV instruction bakes ADMIN_PASSWORD=copaw into the image metadata, making it retrievable via docker inspect or docker history --no-trunc. While the comment notes these should be overridden at runtime, the default credential remains exposed.
Consider one of these approaches:
- Remove the default value and require it at runtime (fail-fast if not set).
- Keep the default only for
ADMIN_USERNAMEbut omitADMIN_PASSWORD, letting the app fall back to open-access mode when unset.
Suggested fix (option 2 - no default password)
# Default auth credentials (override at runtime with -e ADMIN_USERNAME=... -e ADMIN_PASSWORD=...)
ENV ADMIN_USERNAME=admin
-ENV ADMIN_PASSWORD=copaw
+# ADMIN_PASSWORD intentionally has no default - set at runtime to enable auth📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Default auth credentials (override at runtime with -e ADMIN_USERNAME=... -e ADMIN_PASSWORD=...) | |
| ENV ADMIN_USERNAME=admin | |
| ENV ADMIN_PASSWORD=copaw | |
| # Default auth credentials (override at runtime with -e ADMIN_USERNAME=... -e ADMIN_PASSWORD=...) | |
| ENV ADMIN_USERNAME=admin | |
| # ADMIN_PASSWORD intentionally has no default - set at runtime to enable auth |
🧰 Tools
🪛 Trivy (0.69.1)
[error] 21-21: Secrets passed via build-args or envs or copied secret files
Possible exposure of secret env "ADMIN_PASSWORD" in ENV
Rule: DS-0031
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/Dockerfile` around lines 19 - 21, Remove the baked-in ADMIN_PASSWORD
from the Dockerfile to avoid exposing credentials: delete or omit the ENV
ADMIN_PASSWORD=copaw line and keep only ENV ADMIN_USERNAME=admin (if desired);
then update the application startup or entrypoint logic (the component that
reads ADMIN_PASSWORD at runtime) to treat an unset ADMIN_PASSWORD as "no
password / open-access" or to fail-fast with a clear error if you prefer
requiring it at runtime, ensuring the runtime check references ADMIN_PASSWORD
when deciding auth mode.
docker-compose.yml
Outdated
Comment on lines
+9
to
+12
| environment: | ||
| - ADMIN_USERNAME=admin | ||
| - ADMIN_PASSWORD=copaw | ||
| - COPAW_PORT=8088 |
There was a problem hiding this comment.
Avoid committing credentials in docker-compose.yml.
Hardcoded credentials will be exposed in version control. Use environment variable interpolation with defaults, allowing users to override via a .env file (which should be gitignored):
Suggested fix
environment:
- - ADMIN_USERNAME=admin
- - ADMIN_PASSWORD=copaw
+ - ADMIN_USERNAME=${ADMIN_USERNAME:-admin}
+ - ADMIN_PASSWORD=${ADMIN_PASSWORD:-}
- COPAW_PORT=8088Then add a .env.example file documenting the variables and ensure .env is in .gitignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 9 - 12, Replace the hardcoded environment
values in docker-compose.yml with environment variable interpolation using
defaults (e.g., change ADMIN_USERNAME=admin to
ADMIN_USERNAME=${ADMIN_USERNAME:-admin}, ADMIN_PASSWORD=copaw to
ADMIN_PASSWORD=${ADMIN_PASSWORD:-copaw}, and COPAW_PORT=8088 to
COPAW_PORT=${COPAW_PORT:-8088}) so credentials can be overridden via a .env
file; add a .env.example documenting ADMIN_USERNAME, ADMIN_PASSWORD, and
COPAW_PORT and ensure .env is listed in .gitignore to avoid committing secrets.
src/copaw/app/_app.py
Outdated
Comment on lines
+61
to
+63
| # Initialize auth from env vars now that working dir is ready | ||
| init_auth_from_env() | ||
|
|
There was a problem hiding this comment.
Do not initialize auth when admin credentials are missing/blank.
init_auth_from_env() is called unconditionally. With current auth bootstrap behavior, this can create predictable default credentials (or even a blank password) instead of true open-access fallback, which is a critical security risk.
🔒 Proposed hardening in startup flow
`@asynccontextmanager`
async def lifespan(app: FastAPI): # pylint: disable=too-many-statements
- # Initialize auth from env vars now that working dir is ready
- init_auth_from_env()
+ # Initialize auth only when explicit non-empty credentials are provided
+ admin_username = os.environ.get("ADMIN_USERNAME", "").strip()
+ admin_password = os.environ.get("ADMIN_PASSWORD", "").strip()
+ if admin_username and admin_password:
+ init_auth_from_env()
+ else:
+ logger.warning(
+ "Authentication disabled: ADMIN_USERNAME/ADMIN_PASSWORD not fully configured",
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/copaw/app/_app.py` around lines 61 - 63, The startup currently calls
init_auth_from_env() unconditionally which can create default or blank admin
credentials; update the bootstrap to only call init_auth_from_env() when the
expected admin credential environment variables are present and non-empty (e.g.,
check for the specific ADMIN_USER/ADMIN_PASSWORD env vars your auth expects) and
skip initialization otherwise to preserve true open-access fallback; locate the
call to init_auth_from_env() in the app startup (function/module where it’s
invoked) and wrap it with a guard that validates the required env values before
invoking init_auth_from_env().
src/copaw/app/auth.py
Outdated
Comment on lines
+52
to
+63
| def _hash_password(password: str, salt: Optional[str] = None) -> tuple[str, str]: | ||
| """Hash password with salt. Returns (hash_hex, salt_hex).""" | ||
| if salt is None: | ||
| salt = secrets.token_hex(16) | ||
| h = hashlib.sha256((salt + password).encode("utf-8")).hexdigest() | ||
| return h, salt | ||
|
|
||
|
|
||
| def verify_password(password: str, stored_hash: str, salt: str) -> bool: | ||
| """Verify password against stored hash.""" | ||
| h, _ = _hash_password(password, salt) | ||
| return hmac.compare_digest(h, stored_hash) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
head -n 70 src/copaw/app/auth.py | tail -n +40Repository: agentscope-ai/CoPaw
Length of output: 1200
🌐 Web query:
OWASP password hashing recommendations 2024 2025 PBKDF2 iterations
💡 Result:
OWASP’s Password Storage Cheat Sheet (current guidance as published on the OWASP Cheat Sheet Series site) recommends PBKDF2 only when you need FIPS-140 / NIST-aligned options, and specifically calls out these minimum PBKDF2 iteration counts: (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA256: 600,000 iterations (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA1: 1,300,000 iterations (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA512: 210,000 iterations (cheatsheetseries.owasp.org)
Also, OWASP’s overall preference for new systems is:
- Argon2id first, 2) scrypt next, and then bcrypt for legacy; PBKDF2 mainly for FIPS constraints. (cheatsheetseries.owasp.org)
Citations:
- 1: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html?utm_source=openai
- 2: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html?utm_source=openai
- 3: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html?utm_source=openai
- 4: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html?utm_source=openai
- 5: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html?utm_source=openai
🏁 Script executed:
# Also check if there are other password-related functions in the file
rg -n "def.*password|def.*hash" src/copaw/app/auth.pyRepository: agentscope-ai/CoPaw
Length of output: 292
Single-iteration SHA-256 is weak for password hashing.
SHA-256 is fast by design, making it vulnerable to brute-force attacks. Password hashing should use a slow, adaptive algorithm. Per OWASP's current guidance, the preferred algorithms are Argon2id, scrypt, or bcrypt; however, for a dependency-free solution using only stdlib, use hashlib.pbkdf2_hmac with a minimum of 600,000 iterations for SHA-256.
Suggested fix using PBKDF2
+# PBKDF2 iterations - OWASP minimum for SHA-256
+_PBKDF2_ITERATIONS = 600_000
+
+
def _hash_password(password: str, salt: Optional[str] = None) -> tuple[str, str]:
- """Hash password with salt. Returns (hash_hex, salt_hex)."""
+ """Hash password with PBKDF2-SHA256. Returns (hash_hex, salt_hex)."""
if salt is None:
salt = secrets.token_hex(16)
- h = hashlib.sha256((salt + password).encode("utf-8")).hexdigest()
+ h = hashlib.pbkdf2_hmac(
+ "sha256",
+ password.encode("utf-8"),
+ salt.encode("utf-8"),
+ _PBKDF2_ITERATIONS,
+ ).hex()
return h, saltThis will increase login latency slightly (~300-500ms) but significantly improves security. Existing auth.json files will need re-initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/copaw/app/auth.py` around lines 52 - 63, Replace the single-iteration
SHA256 in _hash_password and verify_password with PBKDF2-HMAC-SHA256 using a
high iteration count (e.g., 600000); generate a random salt as bytes
(secrets.token_bytes), run hashlib.pbkdf2_hmac('sha256', password_bytes,
salt_bytes, iterations), and return hex-encoded derived key and hex-encoded salt
from _hash_password, then have verify_password recompute the PBKDF2 result from
the provided salt and compare with hmac.compare_digest; update function
signatures/returns for _hash_password and verify_password accordingly and note
that existing auth.json entries must be re-initialized after this change.
Comment on lines
+27
to
+39
| @router.post("/login") | ||
| async def login(req: LoginRequest): | ||
| """Authenticate with username and password.""" | ||
| if not is_auth_enabled(): | ||
| return {"token": "", "username": "", "message": "Auth not enabled"} | ||
|
|
||
| token = authenticate(req.username, req.password) | ||
| if token is None: | ||
| from fastapi import HTTPException | ||
|
|
||
| raise HTTPException(status_code=401, detail="Invalid credentials") | ||
|
|
||
| return LoginResponse(token=token, username=req.username) |
There was a problem hiding this comment.
Response type mismatch when auth is disabled.
Line 31 returns a dict with message field, but LoginResponse (lines 18-20) doesn't declare it. The frontend's LoginResponse interface includes message?: string, so it works, but the backend model is inconsistent.
Consider adding the optional field to the Pydantic model or returning the model instance:
Option 1: Add message to LoginResponse
class LoginResponse(BaseModel):
token: str
username: str
+ message: str | None = NoneOption 2: Return model instance
if not is_auth_enabled():
- return {"token": "", "username": "", "message": "Auth not enabled"}
+ return LoginResponse(token="", username="")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @router.post("/login") | |
| async def login(req: LoginRequest): | |
| """Authenticate with username and password.""" | |
| if not is_auth_enabled(): | |
| return {"token": "", "username": "", "message": "Auth not enabled"} | |
| token = authenticate(req.username, req.password) | |
| if token is None: | |
| from fastapi import HTTPException | |
| raise HTTPException(status_code=401, detail="Invalid credentials") | |
| return LoginResponse(token=token, username=req.username) | |
| `@router.post`("/login") | |
| async def login(req: LoginRequest): | |
| """Authenticate with username and password.""" | |
| if not is_auth_enabled(): | |
| return LoginResponse(token="", username="") | |
| token = authenticate(req.username, req.password) | |
| if token is None: | |
| from fastapi import HTTPException | |
| raise HTTPException(status_code=401, detail="Invalid credentials") | |
| return LoginResponse(token=token, username=req.username) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/copaw/app/routers/auth.py` around lines 27 - 39, The login handler
returns a plain dict with a message when auth is disabled which doesn't match
the Pydantic LoginResponse model; either update the LoginResponse model to
include an optional message: str | None (or Optional[str]) so the model matches
the frontend, or change the early return in login to return a LoginResponse
instance (e.g., LoginResponse(token="", username="", message="Auth not
enabled")) so the response type is consistent; locate the LoginResponse model
definition and the login function to apply one of these fixes.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
rowanchen-com
added a commit
to rowanchen-com/CoPaw
that referenced
this pull request
Mar 3, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/copaw/app/auth.py (1)
52-57:⚠️ Potential issue | 🟠 MajorUse an adaptive password KDF instead of single-pass SHA-256 (still unresolved).
At Line 56, password hashing is still a fast single-pass SHA-256, which is weak for stored credentials if
auth.jsonis exposed. This was already raised in prior review and remains open.Also applies to: 60-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/auth.py` around lines 52 - 57, Replace the single-pass SHA-256 in _hash_password with a slow, adaptive KDF (e.g., bcrypt or Argon2) to harden stored credentials: change _hash_password to call the chosen KDF (generate a per-password salt via the KDF or secrets.token_bytes if required), store the KDF output (and any KDF parameters) instead of raw SHA hex, and update any verification code that relies on _hash_password to use the KDF's verify function (look for calls to _hash_password and related verification logic around lines 60-63) so password checks use the KDF's verify API rather than comparing SHA256 digests.
🧹 Nitpick comments (1)
console/src/App.tsx (1)
61-64: Consider preserving the full URL including query string and hash.The redirect only preserves
window.location.pathname. If users were at a URL like/chat?tab=settings#section, they would lose the query parameters and hash after login.♻️ Suggested enhancement to preserve full path
if (status === "auth-required") - return <Navigate to={`/login?redirect=${encodeURIComponent(window.location.pathname)}`} replace />; + return <Navigate to={`/login?redirect=${encodeURIComponent(window.location.pathname + window.location.search + window.location.hash)}`} replace />;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@console/src/App.tsx` around lines 61 - 64, The redirect currently uses window.location.pathname so query string and hash are lost; update the Navigate redirect to include the full path by concatenating window.location.pathname + window.location.search + window.location.hash (and encodeURIComponent the result) instead of just window.location.pathname — locate the conditional that checks status (status === "auth-required") and the Navigate usage and replace the redirect value so post-login returns users to the exact original URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/copaw/app/auth.py`:
- Around line 160-163: Docstring and implementation disagree: the module-level
docstring claims removing ADMIN_PASSWORD disables auth, but initialize_auth()
(the startup routine that checks ADMIN_PASSWORD and manages auth.json) currently
only skips re-initialization and leaves existing credentials intact. Fix by
implementing the described disable flow in initialize_auth(): when
os.getenv("ADMIN_PASSWORD") is None or empty, explicitly remove or rename the
existing auth store (auth.json) to fully disable auth (ensure safe handling:
check file existence, back it up or remove, and log the action), or
alternatively update the module docstring to accurately state that removing
ADMIN_PASSWORD only prevents re-initialization and does not delete existing
auth.json—pick one and apply consistently across the module-level docstring and
the initialize_auth() behavior.
- Around line 127-135: Currently _load_auth_data() swallows read/parse errors
and returns {}, which makes is_auth_enabled() treat auth as disabled and lets
the auth middleware bypass protection; change _load_auth_data to surface errors
(raise or return a sentinel like None on failure) instead of returning an empty
dict, update is_auth_enabled() to treat a failed/unknown auth read as "auth
enabled" (fail-closed) or return an error state, and modify the auth middleware
to detect that error/None state and respond with a 503 (or other hard-fail
response) rather than allowing access. Reference _load_auth_data(),
is_auth_enabled(), and the auth middleware to implement these changes.
- Around line 119-120: Replace the broad "except Exception: return None" in the
token verification block with explicit handling of expected decode/validation
errors (e.g., jwt.ExpiredSignatureError, jwt.DecodeError, ValueError,
json.JSONDecodeError) so those return None, and add a separate except Exception
as e that logs the unexpected exception (using the module's logger) and then
re-raises or returns None as appropriate; locate the token decode/verify
function (the token verification block) and update its exception clauses to
catch specific exceptions first, log error details for unexpected errors, and
avoid swallowing all exceptions silently.
- Around line 256-264: Currently the code falls back to
request.query_params.get("token") for any request; change it so the query-param
fallback is only used for explicit WebSocket handshakes. Keep the Authorization
header parsing (auth_header.startswith("Bearer ")) and only assign token =
request.query_params.get("token") when the request is a WebSocket handshake
(detectable by headers like Upgrade == "websocket" or presence of
"Sec-WebSocket-Key" on the request). Update the block around auth_header / token
(the token variable and auth_header handling in auth.py) to require that
WebSocket-specific condition before reading the query param.
---
Duplicate comments:
In `@src/copaw/app/auth.py`:
- Around line 52-57: Replace the single-pass SHA-256 in _hash_password with a
slow, adaptive KDF (e.g., bcrypt or Argon2) to harden stored credentials: change
_hash_password to call the chosen KDF (generate a per-password salt via the KDF
or secrets.token_bytes if required), store the KDF output (and any KDF
parameters) instead of raw SHA hex, and update any verification code that relies
on _hash_password to use the KDF's verify function (look for calls to
_hash_password and related verification logic around lines 60-63) so password
checks use the KDF's verify API rather than comparing SHA256 digests.
---
Nitpick comments:
In `@console/src/App.tsx`:
- Around line 61-64: The redirect currently uses window.location.pathname so
query string and hash are lost; update the Navigate redirect to include the full
path by concatenating window.location.pathname + window.location.search +
window.location.hash (and encodeURIComponent the result) instead of just
window.location.pathname — locate the conditional that checks status (status ===
"auth-required") and the Navigate usage and replace the redirect value so
post-login returns users to the exact original URL.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/copaw/app/auth.py (2)
52-57:⚠️ Potential issue | 🟠 MajorReplace single-round SHA-256 with a slow password KDF.
Line 56 still uses fast SHA-256 for password hashing, which is too weak against offline brute-force if
auth.jsonis exposed.🔧 Suggested hardening (PBKDF2-SHA256)
+# OWASP-aligned baseline for PBKDF2-SHA256 +_PBKDF2_ITERATIONS = 600_000 + def _hash_password(password: str, salt: Optional[str] = None) -> tuple[str, str]: - """Hash password with salt. Returns (hash_hex, salt_hex).""" + """Hash password with PBKDF2-SHA256. Returns (hash_hex, salt_hex).""" if salt is None: - salt = secrets.token_hex(16) - h = hashlib.sha256((salt + password).encode("utf-8")).hexdigest() + salt = secrets.token_hex(16) + h = hashlib.pbkdf2_hmac( + "sha256", + password.encode("utf-8"), + salt.encode("utf-8"), + _PBKDF2_ITERATIONS, + ).hex() return h, salt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/auth.py` around lines 52 - 57, The _hash_password function is using a single SHA-256 round which is too fast; replace it with a slow KDF (e.g., hashlib.pbkdf2_hmac with 'sha256' and a high iteration count like 100_000) and generate a cryptographically-random salt (use secrets.token_bytes or token_hex) and return the derived-key hex and salt hex as before; ensure the same parameters (algorithm, iterations, salt decoding) are used in any verification routine that consumes _hash_password outputs so stored hashes can be validated.
271-279:⚠️ Potential issue | 🟠 MajorTighten query-token fallback to explicit WebSocket handshake only.
Line 278 checks
Connectionfor"upgrade"; that is still too broad and allows query-param bearer tokens on non-WebSocket API traffic.🔒 Narrow fallback scope
if auth_header.startswith("Bearer "): token = auth_header[7:] - elif "upgrade" in request.headers.get("connection", "").lower(): + elif request.headers.get("upgrade", "").lower() == "websocket": token = request.query_params.get("token")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/auth.py` around lines 271 - 279, The fallback that reads token from request.query_params should only run for real WebSocket handshakes; change the condition that currently checks elif "upgrade" in request.headers.get("connection", "").lower(): to an explicit WebSocket check (for example verify request.scope.get("type") == "websocket" or check that request.headers.get("upgrade", "").lower() == "websocket") before assigning token = request.query_params.get("token"); keep the existing Authorization header parsing (auth_header and token) intact.
🧹 Nitpick comments (1)
src/copaw/app/auth.py (1)
139-141: Preferlogger.exceptionin this handler.At Line 140,
logger.exception(...)gives traceback context automatically and improves operability during incident debugging.🛠️ Small logging improvement
- except (json.JSONDecodeError, OSError) as exc: - logger.error("Failed to load auth file %s: %s", AUTH_FILE, exc) + except (json.JSONDecodeError, OSError): + logger.exception("Failed to load auth file %s", AUTH_FILE) return {"_auth_load_error": True}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copaw/app/auth.py` around lines 139 - 141, The exception handler that catches json.JSONDecodeError and OSError uses logger.error and returns {"_auth_load_error": True}; replace the logger.error call with logger.exception so the stack trace is recorded automatically (i.e., in the except block handling JSONDecodeError/OSError referencing AUTH_FILE, call logger.exception(...) instead of logger.error(...)) while keeping the same return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/copaw/app/auth.py`:
- Around line 52-57: The _hash_password function is using a single SHA-256 round
which is too fast; replace it with a slow KDF (e.g., hashlib.pbkdf2_hmac with
'sha256' and a high iteration count like 100_000) and generate a
cryptographically-random salt (use secrets.token_bytes or token_hex) and return
the derived-key hex and salt hex as before; ensure the same parameters
(algorithm, iterations, salt decoding) are used in any verification routine that
consumes _hash_password outputs so stored hashes can be validated.
- Around line 271-279: The fallback that reads token from request.query_params
should only run for real WebSocket handshakes; change the condition that
currently checks elif "upgrade" in request.headers.get("connection",
"").lower(): to an explicit WebSocket check (for example verify
request.scope.get("type") == "websocket" or check that
request.headers.get("upgrade", "").lower() == "websocket") before assigning
token = request.query_params.get("token"); keep the existing Authorization
header parsing (auth_header and token) intact.
---
Nitpick comments:
In `@src/copaw/app/auth.py`:
- Around line 139-141: The exception handler that catches json.JSONDecodeError
and OSError uses logger.error and returns {"_auth_load_error": True}; replace
the logger.error call with logger.exception so the stack trace is recorded
automatically (i.e., in the except block handling JSONDecodeError/OSError
referencing AUTH_FILE, call logger.exception(...) instead of logger.error(...))
while keeping the same return value.
Author
|
Renderings |
Author
|
Just wanted to check in on this PR. I've addressed all the issues raised by both Qodo and CodeRabbit bots, and there are currently no outstanding review comments. I noticed there are now 4 file conflicts due to recent upstream changes. I'm happy to rebase and resolve them, but as the upstream repo keeps evolving, unmerged PRs will inevitably accumulate more conflicts over time. Could you let me know if there are any concerns or changes you'd like me to make? I'd love to get this moving forward. Thanks! 🙏 |
rowanchen-com
changed the title
rowanchen-com
added a commit
to rowanchen-com/CoPaw
that referenced
this pull request
Mar 6, 2026
Author
|
Rebased onto the latest upstream/main — all merge conflicts have been resolved. Ready for review. |
Author
|
Preview of the login UI and sidebar logout: |
Member
|
Awesome work. Note: |
Author
Thanks for the heads up! I've reviewed the CLI HTTP client — it calls /api/ endpoints without auth headers, so CLI commands would get 401 when auth is enabled. Two options to handle this:
Which approach do you prefer? Or is there another pattern you'd like to follow? |
Author
|
Hi @zhijianma @rayrayraykk, just following up on this — I've tested the CLI ( I'm leaning toward Option 1: bypass auth for localhost requests — the CLI only runs on 127.0.0.1 so it's safe, and it requires zero changes to the CLI code or user workflow. I can have this ready quickly. Let me know if that works, or if you'd prefer a different approach. Happy to implement either way! |
rowanchen-com
added a commit
to rowanchen-com/CoPaw
that referenced
this pull request
Mar 8, 2026
Author
|
@zhijianma @rayrayraykk Updated! I've rebased onto the latest main and added a fix for CLI compatibility. The CLI ( Changes in this update: Rebased onto latest upstream/main (resolved Dockerfile conflict) |
Member
|
We will review this pr soon, thank you |
10 tasks
rowanchen-com
added a commit
to rowanchen-com/CoPaw
that referenced
this pull request
Mar 11, 2026
rowanchen-com
requested a deployment
to
maintainer-approved
GitHub Actions
Waiting
rayrayraykk
reviewed
Mar 11, 2026
Member
rayrayraykk
left a comment
rayrayraykk
left a comment
There was a problem hiding this comment.
The login page should be disabled by default and only enabled via an environment variable. It should also support proper user registration, rather than relying solely on environment-variable–based credentials; otherwise, an agent could easily steal or abuse those credentials. Please refactor your implementation.
docker-compose.yml
Outdated
| @@ -0,0 +1,15 @@ | |||
| version: "3.8" | |||
Member
There was a problem hiding this comment.
This file is beyond the scope of this PR. If you want to tell users how to use, please add to docs.
deploy/Dockerfile
Outdated
| ENV COPAW_WORKING_DIR=/app/working | ||
| ENV COPAW_SECRET_DIR=/app/working.secret | ||
|
|
||
| # Default auth credentials (override at runtime with -e ADMIN_USERNAME=... -e ADMIN_PASSWORD=...) |
Member
There was a problem hiding this comment.
Please do not modify this file.
rayrayraykk
reviewed
Mar 11, 2026
| "agentConfig": { | ||
| "title": "Configuration", | ||
| "description": "Configure agent runtime parameters", | ||
| "reactAgentTitle": "ReAct Agent", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds an optional authentication system and login page for the Web UI to prevent unauthorized access on public-facing deployments.
Key changes:
hashlib,hmac,secrets) — zero new backend dependencies.auth.json(persisted in the Docker-mapped working directory).localStorage.ADMIN_PASSWORDenvironment variable is explicitly set. Without it, the app behaves exactly like upstream — no login required.0600permissions; auth state fails closed if the file is unreadable.OPTIONS) requests pass through the auth middleware./api/auth/verifyendpoint for token validation in the frontend AuthGuard.Related Issue: Fixes #492
Security Considerations:
ADMIN_USERNAME/ADMIN_PASSWORD), not from a.envfile — no hardcoded secrets.auth.jsoncannot be read, auth is treated as enabled rather than bypassed.Type of Change
Component(s) Affected
Checklist
pre-commit run --all-filesor CI)pytestor as relevant)Testing
docker compose up— Dockerfile setsADMIN_USERNAME=adminandADMIN_PASSWORD=copaw. Navigate to the web console, you should see the login page.copaw appwithout settingADMIN_PASSWORD. The app behaves exactly like upstream — no login page, direct access.ADMIN_PASSWORD=your_passwordas an environment variable, restart the app. Login page appears.ADMIN_PASSWORD, restart. All existing sessions are invalidated, users must re-login.Additional Notes
auth.json) is stored in the Docker volume-mapped working directory (/app/working), persisted across container rebuilds.