add a page for tickets from gitea #1

blakeridgway · 2026-04-11T13:15:47-05:00

blakeridgway commented

2026-04-11 13:15:47 -05:00

No description provided.

blakeridgway added 1 commit 2026-04-11 13:15:49 -05:00

add a page for tickets from gitea 0ba2f7af4e

blakeridgway commented

2026-04-11 13:16:31 -05:00

AI Code Review

Code Review

Bugs & Logic Errors

internal/gitea/gitea.go:115 – Silent error handling:
json.NewDecoder(resp.Body).Decode(&labels) is followed by //nolint:errcheck, which ignores a potential JSON decoding error. If the response body is malformed, the function will proceed as if no labels exist, potentially creating duplicate labels. This should be handled properly.
internal/gitea/gitea.go:124 – Hardcoded label color:
The label color #e11d48 is hardcoded in adminOutagesPost. While the GetOrCreateLabel function accepts a color parameter, the caller always passes the same color. This could be made configurable via an environment variable (e.g., GITEA_LABEL_COLOR) for consistency with other configs.
internal/handler/public.go:196 – Error logging may expose sensitive info:
log.Printf("status: gitea planned outages: %v", err) could log the full error, which might include tokens or internal URLs. Consider logging a sanitized message or using a structured logger that redacts sensitive data.

Security

internal/gitea/gitea.go:71 – Token exposure in logs:
The newRequest method adds Authorization: token ... to headers. If HTTP request logging is enabled elsewhere (e.g., via middleware), the token could be logged. Ensure no debug logging captures full request headers.
templates/admin/outages.html:29 – Open redirect risk:
The issue HTMLURL is rendered as a link with target="_blank". While rel="noopener" is present, the URL comes from an external API (Gitea). If the Gitea instance were compromised, malicious URLs could be injected. This is low risk but worth noting.
internal/handler/admin.go:601 – Missing CSRF protection:
The adminOutagesPost handler modifies Gitea issues via POST requests but does not include CSRF tokens. Since the admin area requires authentication, CSRF protection should be added to prevent forged requests.

Performance

internal/handler/public.go:193 – Unbounded issue limit:
ListIssuesByLabel fetches up to 50 issues, but there’s no pagination. If the repo has many labeled issues, only the first 50 will appear. Consider adding pagination or increasing the limit if needed.
internal/gitea/gitea.go:113 – Inefficient label lookup:
GetOrCreateLabel fetches all labels from the repo each time it’s called. For frequent operations, this could be cached (e.g., with a TTL) to reduce API calls.

Style & Maintainability

internal/gitea/gitea.go:60-68 – Duplicate request creation logic:
The newRequest method handles body != nil and body == nil cases separately, but http.NewRequest accepts nil for the body. The logic can be simplified to a single call.
internal/handler/handler.go:48-50 – Inconsistent env var handling:
GITEA_URL and GITEA_TOKEN are read directly via os.Getenv, while other configs use getenv with defaults. Consider using getenv consistently for clarity.
internal/handler/admin.go:570 – Magic string for label color:
The color #e11d48 appears directly in the handler. Define it as a constant or config variable to make it easier to change.
internal/gitea/gitea.go:184 – HTTP status code check:
RemoveLabel checks resp.StatusCode >= 300 && resp.StatusCode != http.StatusNoContent. The Gitea API may return other success codes (e.g., 200). Consider checking for resp.StatusCode >= 200 && resp.StatusCode < 300 instead, unless the API docs specify exact codes.

Unfinished / Unclear

Missing error handling for Gitea client initialization:
If GITEA_URL is set but GITEA_TOKEN is empty, gitea.New will be called with an empty token, leading to authentication failures. Consider validating required env vars on startup or lazily when first used.
No documentation for rate limits or API expectations:
The code assumes Gitea’s API behaves a certain way (e.g., label creation returns the label object). Add comments referencing the Gitea API version or document assumptions.
data/status.json and data/uptime.json changes:
These files appear to have test data changes (dates in 2026). Ensure these are intentional and not accidental commits.

Summary

The PR adds a useful Gitea integration for tagging planned outages. The main issues are:

Silent JSON decoding error in GetOrCreateLabel
Missing CSRF protection in admin POST endpoint
Hardcoded label color
Inefficient label fetching

Recommend addressing the bug in GetOrCreateLabel and adding CSRF protection before merging. The performance and style issues can be improved in follow-up PRs.

Reviewed by deepseek-coder

## AI Code Review ## Code Review ### Bugs & Logic Errors 1. **`internal/gitea/gitea.go:115` – Silent error handling**: `json.NewDecoder(resp.Body).Decode(&labels)` is followed by `//nolint:errcheck`, which ignores a potential JSON decoding error. If the response body is malformed, the function will proceed as if no labels exist, potentially creating duplicate labels. This should be handled properly. 2. **`internal/gitea/gitea.go:124` – Hardcoded label color**: The label color `#e11d48` is hardcoded in `adminOutagesPost`. While the `GetOrCreateLabel` function accepts a color parameter, the caller always passes the same color. This could be made configurable via an environment variable (e.g., `GITEA_LABEL_COLOR`) for consistency with other configs. 3. **`internal/handler/public.go:196` – Error logging may expose sensitive info**: `log.Printf("status: gitea planned outages: %v", err)` could log the full error, which might include tokens or internal URLs. Consider logging a sanitized message or using a structured logger that redacts sensitive data. ### Security 4. **`internal/gitea/gitea.go:71` – Token exposure in logs**: The `newRequest` method adds `Authorization: token ...` to headers. If HTTP request logging is enabled elsewhere (e.g., via middleware), the token could be logged. Ensure no debug logging captures full request headers. 5. **`templates/admin/outages.html:29` – Open redirect risk**: The issue `HTMLURL` is rendered as a link with `target="_blank"`. While `rel="noopener"` is present, the URL comes from an external API (Gitea). If the Gitea instance were compromised, malicious URLs could be injected. This is low risk but worth noting. 6. **`internal/handler/admin.go:601` – Missing CSRF protection**: The `adminOutagesPost` handler modifies Gitea issues via POST requests but does not include CSRF tokens. Since the admin area requires authentication, CSRF protection should be added to prevent forged requests. ### Performance 7. **`internal/handler/public.go:193` – Unbounded issue limit**: `ListIssuesByLabel` fetches up to 50 issues, but there’s no pagination. If the repo has many labeled issues, only the first 50 will appear. Consider adding pagination or increasing the limit if needed. 8. **`internal/gitea/gitea.go:113` – Inefficient label lookup**: `GetOrCreateLabel` fetches *all* labels from the repo each time it’s called. For frequent operations, this could be cached (e.g., with a TTL) to reduce API calls. ### Style & Maintainability 9. **`internal/gitea/gitea.go:60-68` – Duplicate request creation logic**: The `newRequest` method handles `body != nil` and `body == nil` cases separately, but `http.NewRequest` accepts `nil` for the body. The logic can be simplified to a single call. 10. **`internal/handler/handler.go:48-50` – Inconsistent env var handling**: `GITEA_URL` and `GITEA_TOKEN` are read directly via `os.Getenv`, while other configs use `getenv` with defaults. Consider using `getenv` consistently for clarity. 11. **`internal/handler/admin.go:570` – Magic string for label color**: The color `#e11d48` appears directly in the handler. Define it as a constant or config variable to make it easier to change. 12. **`internal/gitea/gitea.go:184` – HTTP status code check**: `RemoveLabel` checks `resp.StatusCode >= 300 && resp.StatusCode != http.StatusNoContent`. The Gitea API may return other success codes (e.g., 200). Consider checking for `resp.StatusCode >= 200 && resp.StatusCode < 300` instead, unless the API docs specify exact codes. ### Unfinished / Unclear 13. **Missing error handling for Gitea client initialization**: If `GITEA_URL` is set but `GITEA_TOKEN` is empty, `gitea.New` will be called with an empty token, leading to authentication failures. Consider validating required env vars on startup or lazily when first used. 14. **No documentation for rate limits or API expectations**: The code assumes Gitea’s API behaves a certain way (e.g., label creation returns the label object). Add comments referencing the Gitea API version or document assumptions. 15. **`data/status.json` and `data/uptime.json` changes**: These files appear to have test data changes (dates in 2026). Ensure these are intentional and not accidental commits. ### Summary The PR adds a useful Gitea integration for tagging planned outages. The main issues are: - Silent JSON decoding error in `GetOrCreateLabel` - Missing CSRF protection in admin POST endpoint - Hardcoded label color - Inefficient label fetching Recommend addressing the bug in `GetOrCreateLabel` and adding CSRF protection before merging. The performance and style issues can be improved in follow-up PRs. --- *Reviewed by deepseek-coder*

blakeridgway closed this pull request

2026-04-11 13:23:04 -05:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: RidgwaySystems/rs_website#1