Sync Devlake from upstream to konflux

[flacatus]

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

• I have read through the Contributing Documentation.
• I have added relevant tests.
• I have added relevant documentation.
• I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

What does this PR do?

Does this close any open issues?

Closes xx

Screenshots

Include any relevant screenshots here.

Other Information

Any other information that is important to this PR.

[qodo-app-for-konflux-ci]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

[qodo-app-for-konflux-ci]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (7) 🔗 Cross-repo conflicts (0)

1. CORS blocks CSRF header 🐞 Bug ≡ Correctness ⭐ New

Description

The PR adds CSRF enforcement via the X-CSRF-Token header, but the server’s CORS configuration does
not allow that header, causing browser preflight failures for credentialed requests from the UI. As
a result, authenticated unsafe requests (POST/PUT/PATCH/DELETE) from config-ui can be blocked by
the browser before reaching the API.

Code

backend/server/api/api.go[R120-129]

+	// Auth chain order matters: REST API key first (its own short-circuit),
+	// then the push API key gate, then OIDC session, then oauth2-proxy header
+	// (only sets USER if not yet set and the forwarded secret matches), then the terminal 401 gate,
+	// finally CSRF on unsafe methods.
	router.Use(RestAuthentication(router, basicRes))
+	router.Use(RequirePushAuthentication(basicRes))
+	router.Use(auth.OIDCAuthentication())
	router.Use(OAuth2ProxyAuthentication(basicRes))
+	router.Use(auth.RequireAuth())
+	router.Use(auth.CSRFProtect())

Relevance

⭐⭐⭐ High

Repo has active CORS maintenance; adding CSRF header to AllowHeaders fits prior CORS fixes in
api.go.

PR-#51
PR-#52
PR-#53

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Backend enables CSRF middleware while CORS only allows Origin/Content-Type/Authorization; meanwhile
the UI is configured to send X-CSRF-Token, which triggers a CORS preflight that will be rejected
without the header being allow-listed.

backend/server/api/api.go[103-112]
backend/server/api/api.go[120-129]
config-ui/src/utils/request.ts[24-31]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
CSRF protection now requires the `X-CSRF-Token` header on unsafe methods, but `cors.Config.AllowHeaders` does not include `X-CSRF-Token`. Browsers will fail CORS preflight for these requests, breaking the UI’s authenticated mutations.

## Issue Context
The frontend Axios client is configured to send `X-CSRF-Token` automatically (double-submit cookie pattern). The backend must advertise this header in CORS responses.

## Fix Focus Areas
- backend/server/api/api.go[103-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unjustified //nolint directives 📘 Rule violation ⚙ Maintainability ⭐ New

Description

New //nolint directives were added without an accompanying justification comment. This violates
linting compliance requirements and makes it harder to audit why lint rules are being suppressed.

Code

backend/plugins/asana/asana.go[26]

+var PluginEntry impl.Asana //nolint

Relevance

⭐⭐ Medium

No historical suggestions found about requiring justification for //nolint in this repo.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 15 requires any //nolint directive to include a justification comment. The cited
lines introduce //nolint suppressions without any justification text.

CLAUDE.md: Go Formatting & Linting: Use gofmt/goimports and golangci-lint; No nolint Without Justification
backend/plugins/asana/asana.go[26-26]
backend/plugins/icla/api/swagger.go[28-28]
backend/plugins/icla/api/swagger.go[44-44]
backend/plugins/taiga/models/connection.go[79-79]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New `//nolint` directives were introduced without justification comments.

## Issue Context
Compliance requires every `//nolint` to include a justification comment explaining why suppression is necessary.

## Fix Focus Areas
- backend/plugins/asana/asana.go[26-26]
- backend/plugins/icla/api/swagger.go[28-28]
- backend/plugins/icla/api/swagger.go[44-44]
- backend/plugins/taiga/models/connection.go[79-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. PluginEntry lacks Godoc 📘 Rule violation ⚙ Maintainability ⭐ New

Description

The new exported identifier PluginEntry is missing a proper Godoc comment. This violates the
requirement to document exported Go identifiers for maintainability and lint compliance.

Code

backend/plugins/asana/asana.go[26]

+var PluginEntry impl.Asana //nolint

Relevance

⭐⭐ Medium

No historical evidence found that missing Godoc on exported identifiers is enforced here.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 22 requires exported identifiers to have Godoc comments starting with the
identifier name. PluginEntry is exported and has no preceding // PluginEntry ... comment.

CLAUDE.md: Document Exported Go Identifiers with Proper Godoc Comments
backend/plugins/asana/asana.go[26-26]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The exported variable `PluginEntry` is missing a Godoc comment.

## Issue Context
Exported identifiers must have comments that start with the identifier name.

## Fix Focus Areas
- backend/plugins/asana/asana.go[26-26]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (3)

4. SessionSecret checked with len() 📘 Rule violation ⚙ Maintainability ⭐ New

Description

New code uses len(s.cfg.SessionSecret) > 0 / == 0 for empty-string checks instead of direct `!=
"" / == ""` comparisons. This violates the required Go string conventions and reduces readability.

Code

backend/server/api/auth/auth.go[360]

+	if len(s.cfg.SessionSecret) > 0 {

Relevance

⭐⭐ Medium

Auth middleware/auth.go paths not found; no historical evidence of enforcing len(s)>0 vs s!=""
style.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 18 explicitly disallows new/modified empty-string checks implemented via `len(s) >
0 / len(s) == 0. The cited lines use len(...) on SessionSecret` to test emptiness.

CLAUDE.md: Go String Conventions: Use strings.EqualFold for Case-Insensitive Comparisons and Prefer Direct Empty-String Checks
backend/server/api/auth/auth.go[360-360]
backend/server/api/auth/middleware.go[67-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
String emptiness is checked via `len(s) > 0` / `len(s) == 0` in new/modified code.

## Issue Context
Compliance requires direct empty-string checks (e.g., `s != ""`) for clarity and idiomatic Go.

## Fix Focus Areas
- backend/server/api/auth/auth.go[360-360]
- backend/server/api/auth/middleware.go[67-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Taiga error includes password 📘 Rule violation ⛨ Security ⭐ New

Description

fetchToken() constructs an error message that includes the serialized request body, which
contains tc.Password. If this error is logged upstream, it can leak credentials into logs.

Code

backend/plugins/taiga/models/connection.go[R70-103]

+	body, e := json.Marshal(map[string]string{
+		"type":     "normal",
+		"username": tc.Username,
+		"password": tc.Password,
+	})
+	if e != nil {
+		return "", errors.Default.WrapRaw(e)
+	}
+
+	resp, e := http.Post(authURL, "application/json", bytes.NewReader(body)) //nolint:noctx
+	if e != nil {
+		return "", errors.Default.WrapRaw(e)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return "", errors.Default.New(fmt.Sprintf("taiga auth failed with status %d", resp.StatusCode))
+	}
+
+	var result map[string]interface{}
+	if e := json.NewDecoder(resp.Body).Decode(&result); e != nil {
+		return "", errors.Default.WrapRaw(e)
+	}
+	// Taiga returns auth_token (v5) or token (v6)
+	for _, key := range []string{"auth_token", "token"} {
+		if t, ok := result[key]; ok {
+			if token, ok := t.(string); ok && token != "" {
+				return token, nil
+			}
+		}
+	}
+	// fallback: read raw body hint
+	raw, _ := io.ReadAll(bytes.NewReader(body))
+	return "", errors.Default.New(fmt.Sprintf("taiga auth response missing token field, body: %s", string(raw)))

Relevance

⭐⭐ Medium

Taiga plugin path not found in repo history; no prior guidance on secrets in errors for that code.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 7 requires error reporting/logging to avoid sensitive data. The request body
includes username and password, and the returned error embeds string(raw) derived from that
body, which can expose credentials.

CLAUDE.md: Log Errors with Context but Without Sensitive Data
backend/plugins/taiga/models/connection.go[70-74]
backend/plugins/taiga/models/connection.go[101-103]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An error path includes `tc.Password` by embedding the marshaled request body in the returned error message.

## Issue Context
Errors are commonly logged by callers; including credentials in error strings risks secret leakage.

## Fix Focus Areas
- backend/plugins/taiga/models/connection.go[70-74]
- backend/plugins/taiga/models/connection.go[101-103]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. CSRF blocks non-cookie auth 🐞 Bug ≡ Correctness

Description

auth.Service.CSRFProtect() enforces CSRF whenever a devlake_session cookie is present, even if
the request was authenticated via OAuth2Proxy headers (or other non-cookie methods), which can cause
valid unsafe requests to fail with 403. This is inconsistent with the middleware’s own comment and
the global middleware ordering (OAuth2ProxyAuthentication runs before CSRFProtect).

Code

backend/server/api/auth/middleware.go[R151-156]

+		// CSRF only applies when the caller is authenticating via the session
+		// cookie. Bearer tokens and proxy headers aren't replayable cross-site.
+		if _, err := c.Cookie(oidchelper.SessionCookieName); err != nil {
+			c.Next()
+			return
+		}

Relevance

⭐⭐ Medium

Auth middleware path not found in this repo; no prior accepted/rejected CSRF/OAuth2Proxy pattern
evidence.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CSRFProtect checks only for the presence of the session cookie to decide if it should enforce
CSRF, but the server installs OAuth2ProxyAuthentication before CSRFProtect, meaning
common.USER can be set via forwarded headers without using the session cookie. This can lead to
CSRF enforcement on requests authenticated through non-cookie mechanisms.

backend/server/api/auth/middleware.go[128-167]
backend/server/api/api.go[120-129]
backend/server/api/middlewares.go[93-116]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CSRFProtect` currently decides to enforce CSRF solely by checking whether the session cookie exists on the request. In the global middleware chain, authentication can be established via non-cookie mechanisms (e.g., `OAuth2ProxyAuthentication`) while the request still carries a stale/irrelevant `devlake_session` cookie, leading to unexpected 403 responses.

### Issue Context
- The code comment says CSRF should apply only when authenticating via the session cookie, but the implementation uses cookie presence as a proxy for “auth via cookie”.
- Since `CSRFProtect` is installed globally after `OAuth2ProxyAuthentication`, a request can be fully authenticated without using the cookie.

### Fix Focus Areas
- backend/server/api/auth/middleware.go[54-96]
- backend/server/api/auth/middleware.go[128-168]
- backend/server/api/api.go[120-129]

### Suggested fix
- In `OIDCAuthentication`, when a session cookie is successfully parsed/validated and used to set `common.USER`, also set a context flag (e.g., `c.Set("auth_via_session_cookie", true)`).
- In `CSRFProtect`, enforce CSRF only when that flag is set (or equivalent signal that cookie-based session auth was actually used for this request), instead of checking only for cookie presence.
- Keep current behavior for unauthenticated/public routes and safe HTTP methods.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. OIDC login log includes email 📘 Rule violation ⛨ Security ⭐ New

Description

The new OIDC login log line includes email and sub, which are potentially sensitive identifiers.
This increases the risk of PII exposure in logs.

Code

backend/server/api/auth/auth.go[R335-337]

+	oidchelper.SetSessionCookie(c, s.cfg, jwt)
+	oidchelper.SetCSRFCookie(c, s.cfg, csrf)
+	s.logger.Info("oidc login: provider=%s sub=%s email=%s jti=%s", state.Provider, sub, email, jti)

Relevance

⭐⭐ Medium

Auth file path not found on default branch; no prior repo-specific decisions about logging
email/sub.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 7 requires logs to avoid sensitive data exposure. The cited log statement emits
email and sub directly into logs.

CLAUDE.md: Log Errors with Context but Without Sensitive Data
backend/server/api/auth/auth.go[337-337]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new log line records `sub` and `email` for OIDC login events.

## Issue Context
Operational logs should include useful context while avoiding sensitive identifiers; consider logging provider and a stable non-PII identifier (or a hashed value) instead.

## Fix Focus Areas
- backend/server/api/auth/auth.go[335-337]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Column validator allows dots 🐞 Bug ≡ Correctness ⭐ New

Description

dal.ValidateColumnName allows dots (.), so user-provided mapping keys like a.b are considered
valid and then interpolated into SQL UPDATE ... SET <field> = ?, which can generate invalid SQL or
unintended qualified identifiers. This can break customize field extraction updates at runtime for
certain mappings.

Code

backend/core/dal/identifier.go[R27-49]

+// validIdentifierRegex matches valid SQL identifiers: alphanumeric, underscores, and dots (for schema.table)
+var validIdentifierRegex = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_.]*$`)
+
+// ValidateTableName checks that a table name is a safe SQL identifier to prevent SQL injection.
+func ValidateTableName(name string) errors.Error {
+	if name == "" {
+		return errors.Default.New("table name must not be empty")
+	}
+	if !validIdentifierRegex.MatchString(name) {
+		return errors.Default.New(fmt.Sprintf("invalid table name: %q", name))
+	}
+	return nil
+}
+
+// ValidateColumnName checks that a column name is a safe SQL identifier to prevent SQL injection.
+func ValidateColumnName(name string) errors.Error {
+	if name == "" {
+		return errors.Default.New("column name must not be empty")
+	}
+	if !validIdentifierRegex.MatchString(name) {
+		return errors.Default.New(fmt.Sprintf("invalid column name: %q", name))
+	}
+	return nil

Relevance

⭐⭐ Medium

File path not present on default branch; no historical review patterns for identifier dot validation
found.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The identifier regex explicitly allows dots and is used by ValidateColumnName. Customize
transformation rules accept arbitrary mapping keys (user-provided) which are validated and then
formatted into SQL field positions via fmt.Sprintf, so dotted keys can pass validation and still
produce invalid UPDATE statements.

backend/core/dal/identifier.go[27-49]
backend/plugins/customize/tasks/task_data.go[20-25]
backend/plugins/customize/tasks/customized_fields_extractor.go[97-103]
backend/plugins/customize/tasks/customized_fields_extractor.go[174-198]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ValidateColumnName` currently uses the same regex as table validation and permits dots. In the customize plugin, mapping keys are treated as column names and are injected directly into `SET`/`WHERE` clauses, so allowing dots can yield malformed SQL (e.g., `SET a.b = ?`).

## Issue Context
Table names may reasonably support `schema.table`, but column names in this call path should be restricted to simple identifiers (letters/digits/underscore) without dots.

## Fix Focus Areas
- backend/core/dal/identifier.go[27-49]
- backend/plugins/customize/tasks/customized_fields_extractor.go[174-198]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Unbounded lastSeen map 🐞 Bug ☼ Reliability

Description

auth.Service.bumpLastSeen() inserts session JTIs into Service.lastSeen but never deletes
entries, so the map can grow without bound over the process lifetime as new sessions are created.
This can degrade reliability over time in long-running/high-churn deployments due to increasing
memory use.

Code

backend/server/api/auth/auth.go[R445-453]

+func (s *Service) bumpLastSeen(jti string) {
+	now := time.Now()
+	s.lastSeenMu.Lock()
+	if last, ok := s.lastSeen[jti]; ok && now.Sub(last) < lastSeenThrottle {
+		s.lastSeenMu.Unlock()
+		return
+	}
+	s.lastSeen[jti] = now
+	s.lastSeenMu.Unlock()

Relevance

⭐⭐ Medium

Auth file path not found in this repo; no historical evidence this team changes/maintains that area
here.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Service struct owns a lastSeen map, and bumpLastSeen only checks/updates it and spawns a
DB update; there is no code path that removes old keys, so keys accumulate over time.

backend/server/api/auth/auth.go[64-76]
backend/server/api/auth/auth.go[443-460]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Service.lastSeen` is used to throttle DB writes for session activity, but it has no eviction strategy. Every new session JTI adds a new key that remains forever.

### Issue Context
The map’s purpose is throttling (`lastSeenThrottle`), so it only needs to remember JTIs for a bounded window. Keeping entries indefinitely is unnecessary and can lead to memory accumulation.

### Fix Focus Areas
- backend/server/api/auth/auth.go[67-76]
- backend/server/api/auth/auth.go[443-460]

### Suggested fix options
- Add TTL-based eviction:
 - After setting `lastSeen[jti]=now`, schedule a delayed cleanup (e.g., `time.AfterFunc(lastSeenThrottle, ...)`) that deletes `jti` **only if** the stored timestamp is unchanged.
 - Or run a periodic sweeper goroutine that deletes entries older than `lastSeenThrottle` (or `2*lastSeenThrottle`).
- Ensure deletion is protected by the same mutex and does not race with updates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Non-plugin files modified 📘 Rule violation § Compliance

Description

This PR modifies files outside the owned plugin directories (backend/plugins/aireview/,
backend/plugins/codecov/, backend/plugins/testregistry/). This violates the requirement to
restrict changes to those owned plugin paths only.

Code

README.md[28]

+[![Slack](https://img.shields.io/badge/slack-join_chat-success.svg?logo=slack)](https://join.slack.com/t/devlake-io/shared_invite/zt-1lkgbdmys-AU2azidzO1u~mtjlg9my7A)

Relevance

⭐ Low

Repo history includes many merged PRs modifying non-owned paths (e.g., root/docs/config-ui) despite
“owned plugins” guidance.

PR-#86
PR-#89
PR-#85

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist restricts modifications to three owned plugin directory prefixes only. The diff shows
a change in README.md, which is outside those allowed directories, so the rule is violated.

Rule 984: Restrict code changes to owned plugin directories only
Rule 1105: Do not modify upstream code outside owned plugin directories
README.md[25-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies files outside the allowed owned plugin directories.

## Issue Context
Compliance requires that all changed files be strictly under `backend/plugins/aireview/`, `backend/plugins/codecov/`, or `backend/plugins/testregistry/`. Any modifications outside these directories must be removed or moved to a separate PR owned by the appropriate team.

## Fix Focus Areas
- README.md[25-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Invalid migration filename prefix 📘 Rule violation ⚙ Maintainability

Description

A new migration script filename uses a 14-digit timestamp prefix (20260509000001_...) instead of
the required YYYYMMDD_description.go format. This can break expected naming/linting conventions
for migrations.

Code

backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[R1-5]

+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0

Relevance

⭐ Low

Repo already uses 14-digit migration prefixes in merged PRs; checklist discussion even suggests
aligning docs to 14-digit convention.

PR-#68
PR-#78
PR-#86

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rule requires migration filenames in models/migrationscripts/ to use an 8-digit YYYYMMDD
date prefix. The added file path shows a 14-digit prefix (20260509000001), which does not match
the required format.

Rule 1108: Migration filenames must follow YYYYMMDD_description.go format
backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[1-5]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The migration script filename does not follow the required `YYYYMMDD_description.go` naming convention.

## Issue Context
Compliance requires migration filenames under `models/migrationscripts/` to be in `YYYYMMDD_description.go` format (8-digit date prefix + lowercase snake_case description).

## Fix Focus Areas
- backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[1-5]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 1fba255

Results up to commit ac9f938

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

1. CSRF blocks non-cookie auth 🐞 Bug ≡ Correctness

Description

auth.Service.CSRFProtect() enforces CSRF whenever a devlake_session cookie is present, even if
the request was authenticated via OAuth2Proxy headers (or other non-cookie methods), which can cause
valid unsafe requests to fail with 403. This is inconsistent with the middleware’s own comment and
the global middleware ordering (OAuth2ProxyAuthentication runs before CSRFProtect).

Code

backend/server/api/auth/middleware.go[R151-156]

+		// CSRF only applies when the caller is authenticating via the session
+		// cookie. Bearer tokens and proxy headers aren't replayable cross-site.
+		if _, err := c.Cookie(oidchelper.SessionCookieName); err != nil {
+			c.Next()
+			return
+		}

Relevance

⭐⭐ Medium

Auth middleware path not found in this repo; no prior accepted/rejected CSRF/OAuth2Proxy pattern
evidence.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

CSRFProtect checks only for the presence of the session cookie to decide if it should enforce
CSRF, but the server installs OAuth2ProxyAuthentication before CSRFProtect, meaning
common.USER can be set via forwarded headers without using the session cookie. This can lead to
CSRF enforcement on requests authenticated through non-cookie mechanisms.

backend/server/api/auth/middleware.go[128-167]
backend/server/api/api.go[120-129]
backend/server/api/middlewares.go[93-116]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CSRFProtect` currently decides to enforce CSRF solely by checking whether the session cookie exists on the request. In the global middleware chain, authentication can be established via non-cookie mechanisms (e.g., `OAuth2ProxyAuthentication`) while the request still carries a stale/irrelevant `devlake_session` cookie, leading to unexpected 403 responses.

### Issue Context
- The code comment says CSRF should apply only when authenticating via the session cookie, but the implementation uses cookie presence as a proxy for “auth via cookie”.
- Since `CSRFProtect` is installed globally after `OAuth2ProxyAuthentication`, a request can be fully authenticated without using the cookie.

### Fix Focus Areas
- backend/server/api/auth/middleware.go[54-96]
- backend/server/api/auth/middleware.go[128-168]
- backend/server/api/api.go[120-129]

### Suggested fix
- In `OIDCAuthentication`, when a session cookie is successfully parsed/validated and used to set `common.USER`, also set a context flag (e.g., `c.Set("auth_via_session_cookie", true)`).
- In `CSRFProtect`, enforce CSRF only when that flag is set (or equivalent signal that cookie-based session auth was actually used for this request), instead of checking only for cookie presence.
- Keep current behavior for unauthenticated/public routes and safe HTTP methods.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unbounded lastSeen map 🐞 Bug ☼ Reliability

Description

auth.Service.bumpLastSeen() inserts session JTIs into Service.lastSeen but never deletes
entries, so the map can grow without bound over the process lifetime as new sessions are created.
This can degrade reliability over time in long-running/high-churn deployments due to increasing
memory use.

Code

backend/server/api/auth/auth.go[R445-453]

+func (s *Service) bumpLastSeen(jti string) {
+	now := time.Now()
+	s.lastSeenMu.Lock()
+	if last, ok := s.lastSeen[jti]; ok && now.Sub(last) < lastSeenThrottle {
+		s.lastSeenMu.Unlock()
+		return
+	}
+	s.lastSeen[jti] = now
+	s.lastSeenMu.Unlock()

Relevance

⭐⭐ Medium

Auth file path not found in this repo; no historical evidence this team changes/maintains that area
here.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Service struct owns a lastSeen map, and bumpLastSeen only checks/updates it and spawns a
DB update; there is no code path that removes old keys, so keys accumulate over time.

backend/server/api/auth/auth.go[64-76]
backend/server/api/auth/auth.go[443-460]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Service.lastSeen` is used to throttle DB writes for session activity, but it has no eviction strategy. Every new session JTI adds a new key that remains forever.

### Issue Context
The map’s purpose is throttling (`lastSeenThrottle`), so it only needs to remember JTIs for a bounded window. Keeping entries indefinitely is unnecessary and can lead to memory accumulation.

### Fix Focus Areas
- backend/server/api/auth/auth.go[67-76]
- backend/server/api/auth/auth.go[443-460]

### Suggested fix options
- Add TTL-based eviction:
 - After setting `lastSeen[jti]=now`, schedule a delayed cleanup (e.g., `time.AfterFunc(lastSeenThrottle, ...)`) that deletes `jti` **only if** the stored timestamp is unchanged.
 - Or run a periodic sweeper goroutine that deletes entries older than `lastSeenThrottle` (or `2*lastSeenThrottle`).
- Ensure deletion is protected by the same mutex and does not race with updates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Non-plugin files modified 📘 Rule violation § Compliance

Description

This PR modifies files outside the owned plugin directories (backend/plugins/aireview/,
backend/plugins/codecov/, backend/plugins/testregistry/). This violates the requirement to
restrict changes to those owned plugin paths only.

Code

README.md[28]

+[![Slack](https://img.shields.io/badge/slack-join_chat-success.svg?logo=slack)](https://join.slack.com/t/devlake-io/shared_invite/zt-1lkgbdmys-AU2azidzO1u~mtjlg9my7A)

Relevance

⭐ Low

Repo history includes many merged PRs modifying non-owned paths (e.g., root/docs/config-ui) despite
“owned plugins” guidance.

PR-#86
PR-#89
PR-#85

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The checklist restricts modifications to three owned plugin directory prefixes only. The diff shows
a change in README.md, which is outside those allowed directories, so the rule is violated.

Rule 984: Restrict code changes to owned plugin directories only
Rule 1105: Do not modify upstream code outside owned plugin directories
README.md[25-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies files outside the allowed owned plugin directories.

## Issue Context
Compliance requires that all changed files be strictly under `backend/plugins/aireview/`, `backend/plugins/codecov/`, or `backend/plugins/testregistry/`. Any modifications outside these directories must be removed or moved to a separate PR owned by the appropriate team.

## Fix Focus Areas
- README.md[25-34]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Invalid migration filename prefix 📘 Rule violation ⚙ Maintainability

Description

A new migration script filename uses a 14-digit timestamp prefix (20260509000001_...) instead of
the required YYYYMMDD_description.go format. This can break expected naming/linting conventions
for migrations.

Code

backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[R1-5]

+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements.  See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0

Relevance

⭐ Low

Repo already uses 14-digit migration prefixes in merged PRs; checklist discussion even suggests
aligning docs to 14-digit convention.

PR-#68
PR-#78
PR-#86

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The rule requires migration filenames in models/migrationscripts/ to use an 8-digit YYYYMMDD
date prefix. The added file path shows a 14-digit prefix (20260509000001), which does not match
the required format.

Rule 1108: Migration filenames must follow YYYYMMDD_description.go format
backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[1-5]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The migration script filename does not follow the required `YYYYMMDD_description.go` naming convention.

## Issue Context
Compliance requires migration filenames under `models/migrationscripts/` to be in `YYYYMMDD_description.go` format (8-digit date prefix + lowercase snake_case description).

## Fix Focus Areas
- backend/plugins/asana/models/migrationscripts/20260509000001_encrypt_connection_token.go[1-5]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-app-for-konflux-ci]

1. Csrf blocks non-cookie auth 🐞 Bug ≡ Correctness

auth.Service.CSRFProtect() enforces CSRF whenever a devlake_session cookie is present, even if
the request was authenticated via OAuth2Proxy headers (or other non-cookie methods), which can cause
valid unsafe requests to fail with 403. This is inconsistent with the middleware’s own comment and
the global middleware ordering (OAuth2ProxyAuthentication runs before CSRFProtect).

Agent Prompt

### Issue description
`CSRFProtect` currently decides to enforce CSRF solely by checking whether the session cookie exists on the request. In the global middleware chain, authentication can be established via non-cookie mechanisms (e.g., `OAuth2ProxyAuthentication`) while the request still carries a stale/irrelevant `devlake_session` cookie, leading to unexpected 403 responses.

### Issue Context
- The code comment says CSRF should apply only when authenticating via the session cookie, but the implementation uses cookie presence as a proxy for “auth via cookie”.
- Since `CSRFProtect` is installed globally after `OAuth2ProxyAuthentication`, a request can be fully authenticated without using the cookie.

### Fix Focus Areas
- backend/server/api/auth/middleware.go[54-96]
- backend/server/api/auth/middleware.go[128-168]
- backend/server/api/api.go[120-129]

### Suggested fix
- In `OIDCAuthentication`, when a session cookie is successfully parsed/validated and used to set `common.USER`, also set a context flag (e.g., `c.Set("auth_via_session_cookie", true)`).
- In `CSRFProtect`, enforce CSRF only when that flag is set (or equivalent signal that cookie-based session auth was actually used for this request), instead of checking only for cookie presence.
- Keep current behavior for unauthenticated/public routes and safe HTTP methods.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-app-for-konflux-ci]

Code review by qodo was updated up to the latest commit 1fba255

[qodo-app-for-konflux-ci]

1. Unjustified //nolint directives 📘 Rule violation ⚙ Maintainability

New //nolint directives were added without an accompanying justification comment. This violates
linting compliance requirements and makes it harder to audit why lint rules are being suppressed.

Agent Prompt

## Issue description
New `//nolint` directives were introduced without justification comments.

## Issue Context
Compliance requires every `//nolint` to include a justification comment explaining why suppression is necessary.

## Fix Focus Areas
- backend/plugins/asana/asana.go[26-26]
- backend/plugins/icla/api/swagger.go[28-28]
- backend/plugins/icla/api/swagger.go[44-44]
- backend/plugins/taiga/models/connection.go[79-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-app-for-konflux-ci]

2. pluginentry lacks godoc 📘 Rule violation ⚙ Maintainability

The new exported identifier PluginEntry is missing a proper Godoc comment. This violates the
requirement to document exported Go identifiers for maintainability and lint compliance.

Agent Prompt

## Issue description
The exported variable `PluginEntry` is missing a Godoc comment.

## Issue Context
Exported identifiers must have comments that start with the identifier name.

## Fix Focus Areas
- backend/plugins/asana/asana.go[26-26]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-app-for-konflux-ci]

3. sessionsecret checked with len() 📘 Rule violation ⚙ Maintainability

New code uses len(s.cfg.SessionSecret) > 0 / == 0 for empty-string checks instead of direct `!=
"" / == ""` comparisons. This violates the required Go string conventions and reduces readability.

Agent Prompt

## Issue description
String emptiness is checked via `len(s) > 0` / `len(s) == 0` in new/modified code.

## Issue Context
Compliance requires direct empty-string checks (e.g., `s != ""`) for clarity and idiomatic Go.

## Fix Focus Areas
- backend/server/api/auth/auth.go[360-360]
- backend/server/api/auth/middleware.go[67-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-app-for-konflux-ci]

4. Taiga error includes password 📘 Rule violation ⛨ Security

fetchToken() constructs an error message that includes the serialized request body, which
contains tc.Password. If this error is logged upstream, it can leak credentials into logs.

Agent Prompt

## Issue description
An error path includes `tc.Password` by embedding the marshaled request body in the returned error message.

## Issue Context
Errors are commonly logged by callers; including credentials in error strings risks secret leakage.

## Fix Focus Areas
- backend/plugins/taiga/models/connection.go[70-74]
- backend/plugins/taiga/models/connection.go[101-103]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-app-for-konflux-ci]

5. Cors blocks csrf header 🐞 Bug ≡ Correctness

The PR adds CSRF enforcement via the X-CSRF-Token header, but the server’s CORS configuration does
not allow that header, causing browser preflight failures for credentialed requests from the UI. As
a result, authenticated unsafe requests (POST/PUT/PATCH/DELETE) from config-ui can be blocked by
the browser before reaching the API.

Agent Prompt

## Issue description
CSRF protection now requires the `X-CSRF-Token` header on unsafe methods, but `cors.Config.AllowHeaders` does not include `X-CSRF-Token`. Browsers will fail CORS preflight for these requests, breaking the UI’s authenticated mutations.

## Issue Context
The frontend Axios client is configured to send `X-CSRF-Token` automatically (double-submit cookie pattern). The backend must advertise this header in CORS responses.

## Fix Focus Areas
- backend/server/api/api.go[103-112]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[flacatus]

/retest

[flacatus]

/retest