Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a full Nginx OpenID Connect module: discovery/JWKS caching, JWT verification (many algs), PKCE, session management (memory/Redis), HTTP subrequest helpers, URL/JSON/hash utilities, extensive tests, Docker + CI, and project formatting/config files. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Nginx as Nginx+OIDC
participant IdP as Identity Provider
participant SessionStore as Session Store
participant Metadata as Metadata/JWKS Cache
Client->>Nginx: GET /protected
Nginx->>Nginx: access handler determines request type
alt not authenticated
Nginx->>Metadata: ensure discovery & JWKS
Metadata-->>Nginx: metadata + jwks_uri
Nginx->>SessionStore: store pre-auth (state, nonce, verifier)
Nginx->>Client: 302 Redirect to IdP auth endpoint
Client->>IdP: Authorization request
IdP-->>Client: Redirect to callback (code,state)
Client->>Nginx: GET /callback?code=...&state=...
Nginx->>SessionStore: retrieve pre-auth data
Nginx->>IdP: POST token exchange
IdP-->>Nginx: id_token + access_token
Nginx->>Metadata: fetch JWKS if needed
Metadata-->>Nginx: JWKS keys
Nginx->>Nginx: verify token signature & claims
Nginx->>SessionStore: save tokens and rotate session
Nginx->>Client: 302 Redirect to original URL with cookie
else authenticated
Nginx->>SessionStore: read session tokens
SessionStore-->>Nginx: tokens present/valid
Nginx->>Client: allow request (proxy to app)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150 minutes Poem
Comment |
kjdev
force-pushed
the
develop
branch
6 times, most recently
from
d6f5854 to
6ca17d3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
.github/workflows/package.yaml-107-114 (1)
107-114:⚠️ Potential issue | 🟡 Minor
github.actormay not be the package owner — usegithub.repository_owner.
github.actoris whoever triggered the run (e.g. a collaborator), not necessarily the org/user that owns the GHCR package. This will cause the cleanup to silently fail (masked bycontinue-on-error: true).Proposed fix
- name: Clean up uses: dataaxiom/ghcr-cleanup-action@cd0cdb900b5dbf3a6f2cc869f0dbb0b8211f50c4 `#v1.0.16` with: - owner: ${{ github.actor }} + owner: ${{ github.repository_owner }} package: ${{ github.event.repository.name }}/nginx token: ${{ github.token }} keep-n-untagged: 0 continue-on-error: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/package.yaml around lines 107 - 114, The workflow step "Clean up" uses dataaxiom/ghcr-cleanup-action and incorrectly sets the owner input to `${{ github.actor }}`; change that to `${{ github.repository_owner }}` so the package owner is the repository/org owner (update the owner field in the "Clean up" step), leaving the rest of the inputs (package, token, keep-n-untagged, continue-on-error) unchanged.src/ngx_oidc_handler_logout.c-131-134 (1)
131-134:⚠️ Potential issue | 🟡 MinorUnreachable
metadata == NULLguard at Line 134.
ctx->cached.metadatais already verified non-NULL at Line 122, then assigned tometadataat Line 129, and immediately passed (dereferenced) tongx_oidc_metadata_get_end_session_endpoint(metadata)at Line 131. Themetadata == NULLbranch in the composite condition on Line 134 can never be true.🔧 Suggested fix
- if (metadata == NULL || end_session == NULL || end_session->len == 0) { + if (end_session == NULL || end_session->len == 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_logout.c` around lines 131 - 134, The check for metadata == NULL is unreachable because ctx->cached.metadata was verified non-NULL earlier and metadata is immediately passed to ngx_oidc_metadata_get_end_session_endpoint; either remove metadata == NULL from the composite condition or, alternatively, move the NULL check for metadata before calling ngx_oidc_metadata_get_end_session_endpoint so you only call that function when metadata is non-NULL. Update the condition around end_session (and the surrounding logic that references end_session->len) to rely only on end_session being NULL/empty or perform the metadata NULL guard prior to calling ngx_oidc_metadata_get_end_session_endpoint(metadata).src/ngx_oidc_hash.c-27-62 (1)
27-62:⚠️ Potential issue | 🟡 Minor
hash_computelacks NULL guards forr,input,output, andoutput_len.If any caller passes NULL for these parameters (e.g.,
outputnot allocated large enough), the function will dereference them and crash. The public wrappers (ngx_oidc_hash_sha256etc.) pass parameters through without validation.🛡️ Suggested fix
static ngx_int_t hash_compute(ngx_http_request_t *r, ngx_str_t *input, u_char *output, unsigned int *output_len, const EVP_MD *md, const char *algorithm_name) { + if (r == NULL || input == NULL || output == NULL || output_len == NULL) { + return NGX_ERROR; + } + EVP_MD_CTX *ctx;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_hash.c` around lines 27 - 62, hash_compute currently dereferences r, input, output, and output_len without validation which can crash if callers like ngx_oidc_hash_sha256 pass NULL or an undersized output buffer; add upfront NULL checks in hash_compute for r, input, output, and output_len, log a descriptive ngx_log_error (including algorithm_name) and return NGX_ERROR if any are NULL, and additionally validate that *output_len is at least EVP_MD_size(md) (log and return NGX_ERROR if too small) so callers (e.g., ngx_oidc_hash_sha256) cannot trigger a buffer overrun.src/ngx_oidc_session.c-258-264 (1)
258-264:⚠️ Potential issue | 🟡 MinorFormat specifier mismatch:
%uiused forsize_tvalue.
token_data.lenissize_t, but%uiis forngx_uint_t. Use%uzforsize_tvalues in nginx logging.Proposed fix
} else { ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "oidc_session: moved userinfo to new session " - "(%ui bytes)", token_data.len); + "(%uz bytes)", token_data.len); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session.c` around lines 258 - 264, The ngx_log_debug1 call in the oidc session rotation path uses the wrong format specifier (%ui) for token_data.len (a size_t) — update the format string in the ngx_log_debug1 invocation that logs "oidc_session: moved userinfo to new session" to use %uz instead of %ui so the size_t value token_data.len is logged correctly; locate the ngx_log_debug1 call in the session rotation/oidc_session code and change the specifier only.src/ngx_oidc_session.c-593-626 (1)
593-626:⚠️ Potential issue | 🟡 MinorFragile buffer size: hardcoded
"600"insizeofwill underflow ifNGX_OIDC_PRE_AUTH_TIMEOUTchanges.The
lencalculation usessizeof("; Path=/; HttpOnly; SameSite=Lax; Max-Age=600")which reserves exactly 3 characters for the number "600". IfNGX_OIDC_PRE_AUTH_TIMEOUTis ever changed to a value with more digits (e.g., 3600),ngx_snprintfwill silently truncate the cookie header, producing a malformedSet-Cookie. UseNGX_INT_T_LENfor the numeric portion instead.Proposed fix
/* Calculate Set-Cookie header length */ len = cookie_name.len + sizeof("=") - 1 + cookie_value.len - + sizeof("; Path=/; HttpOnly; SameSite=Lax; Max-Age=600") - 1; + + sizeof("; Path=/; HttpOnly; SameSite=Lax; Max-Age=") - 1 + + NGX_INT_T_LEN; if (secure) { len += sizeof("; Secure") - 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session.c` around lines 593 - 626, The Set-Cookie buffer length calculation is fragile because it hardcodes the digits for NGX_OIDC_PRE_AUTH_TIMEOUT; update the len computation to reserve NGX_INT_T_LEN bytes for the numeric Max-Age instead of using sizeof("...600"), so that when building the header with ngx_snprintf (in the block that formats "%V=%V; Path=/; HttpOnly; ... Max-Age=%d") there is always enough space; adjust the base string length to exclude the "600" literal, add NGX_INT_T_LEN (and still add the Secure suffix when secure is true), and keep set_cookie->value allocation and ngx_snprintf usage unchanged.src/ngx_oidc_handler_authenticate.h-9-22 (1)
9-22:⚠️ Potential issue | 🟡 MinorHeader is not self-contained: include
ngx_http_oidc_module.hfor type definition.The function signature uses
ngx_http_oidc_provider_t *provider(line 22) but this type is defined inngx_http_oidc_module.h, which is not included. Any translation unit that includes only this header will fail to compile. Add#include "ngx_http_oidc_module.h"after the other includes.Suggested fix
`#include` <ngx_config.h> `#include` <ngx_core.h> `#include` <ngx_http.h> +#include "ngx_http_oidc_module.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_authenticate.h` around lines 9 - 22, The header is missing the definition for ngx_http_oidc_provider_t used by ngx_oidc_handler_authenticate; add `#include` "ngx_http_oidc_module.h" to this header (after the existing ngx includes) so the type is available to any translation unit that includes ngx_oidc_handler_authenticate's declaration and avoid compile errors.src/ngx_http_oidc_module.h-12-16 (1)
12-16:⚠️ Potential issue | 🟡 MinorRemove unnecessary
<jansson.h>include.This header includes
<jansson.h>but doesn't use any Jansson types directly. Sincengx_oidc_json.hdeliberately abstracts away Jansson (exporting only an opaquengx_oidc_json_tpointer), the<jansson.h>include should be removed to maintain the abstraction boundary. The requiredngx_oidc_json_ttype (used at line 165) is already available transitively throughngx_oidc_jwt.h.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_http_oidc_module.h` around lines 12 - 16, Remove the unnecessary `#include` <jansson.h> from ngx_http_oidc_module.h: the file does not use any Jansson types directly and relies on the opaque ngx_oidc_json_t which is provided transitively via ngx_oidc_jwt.h, so delete the <jansson.h> include to preserve the abstraction boundary while leaving the existing includes (ngx_oidc_jwt.h, ngx_oidc_session_store.h, ngx_oidc_metadata.h) intact.src/ngx_oidc_json.c-414-432 (1)
414-432:⚠️ Potential issue | 🟡 Minor
ngx_oidc_json_integertruncatesjson_int_t(typicallylong long) tongx_int_t(pointer-sized) on 32-bit platforms.Jansson's
json_integer_valuereturnsjson_int_t, which is defined aslong long(orlong)—typically 64-bit on modern platforms. Casting tongx_int_t(which isintptr_t, i.e., 32-bit on 32-bit platforms) silently truncates large values. This affects OIDC timestamp fields (exp,iat,nbf) on 32-bit systems and creates a latent Y2038 overflow risk on 32-bit platforms. Additionally, inngx_oidc_variable.c:357, the code allocatesNGX_INT64_LEN(64-bit buffer) but stores a 32-bit value, then formats it with"%L"(64-bit specifier)—an inconsistency that masks the underlying truncation issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_json.c` around lines 414 - 432, ngx_oidc_json_integer currently casts Jansson's json_integer_value (json_int_t, typically 64-bit) down to ngx_int_t which truncates on 32-bit platforms; change ngx_oidc_json_integer to return a 64-bit type (e.g. ngx_int64_t or json_int_t) and store json_integer_value into that 64-bit variable without narrowing cast, update the function signature and all call sites (including ngx_oidc_variable.c) to accept the 64-bit return, and in ngx_oidc_variable.c ensure the buffer allocation uses NGX_INT64_LEN and the formatting uses the correct 64-bit specifier (matching NGINX's "%L" / ngx_sprintf expectations) so timestamps (exp/iat/nbf) are preserved on 32-bit platforms..github/workflows/test.yaml-86-100 (1)
86-100:⚠️ Potential issue | 🟡 MinorNo readiness check for the IdP stub container before running tests.
The
docker run --detachreturns immediately. If the stub isn't ready whenprovestarts, tests will fail intermittently. Add a health/readiness poll.Example readiness check
ghcr.io/kjdev/oidc-provider-stub &>/dev/null + # Wait for IdP stub to be ready + for i in $(seq 1 30); do + curl -sf http://127.0.0.1:8888/.well-known/openid-configuration && break + sleep 1 + done # Run Redis🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml around lines 86 - 100, The workflow currently starts the IdP stub container named oidc-stub with docker run --detach but immediately proceeds to run tests, causing race conditions; add a readiness poll after starting the container that waits until the stub is accepting requests (for example, poll an HTTP health or metadata endpoint on localhost:8888 or check docker logs for a "listening" message) with a timeout and short sleep between attempts before running prove -r t; update the job to fail if the container never becomes ready and only then continue to running Redis and the prove command so tests run reliably against a healthy oidc-stub.README.md-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorUpdate README license statement to match the actual MIT license.
Line 53 claims "2-clause BSD License (same as nginx)", but the LICENSE file contains the MIT License. Update the README to reflect the correct license.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 53, Update the README.md license statement to match the project's actual LICENSE file: replace the current "2-clause BSD License (same as nginx)" text with the correct "MIT License" wording (e.g., "License: MIT License" or "MIT License (see LICENSE)"), ensuring the README and LICENSE file are consistent.src/ngx_oidc_handler_logout.h-9-23 (1)
9-23:⚠️ Potential issue | 🟡 MinorMissing include for
ngx_http_oidc_provider_tin handler headers — not self-contained.Both
ngx_oidc_handler_logout.handngx_oidc_handler_authenticate.husengx_http_oidc_provider_t *in their function declarations but do not includengx_http_oidc_module.h, where the type is defined. These headers will fail to compile if included beforengx_http_oidc_module.h. Compare withsrc/ngx_oidc_provider.h, which correctly includes it.Proposed fix
Add to both files after the standard includes:
`#include` <ngx_config.h> `#include` <ngx_core.h> `#include` <ngx_http.h> +#include "ngx_http_oidc_module.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_logout.h` around lines 9 - 23, The headers ngx_oidc_handler_logout.h and ngx_oidc_handler_authenticate.h declare functions using ngx_http_oidc_provider_t* but do not include the header that defines that type; update both files to include ngx_http_oidc_module.h (the header that defines ngx_http_oidc_provider_t) after the existing ngx_config.h/ngx_core.h/ngx_http.h includes so the declarations (e.g., ngx_int_t ngx_oidc_handler_logout(ngx_http_request_t *r, ngx_http_oidc_provider_t *provider);) are self-contained and will compile when these handler headers are included before ngx_http_oidc_module.h.src/ngx_oidc_jwt.h-40-41 (1)
40-41:⚠️ Potential issue | 🟡 MinorDoc comment references
token_typefield that doesn't exist in the struct.The Validation Options section mentions
token_type: Expected typ header value (e.g., "JWT")but the struct has no such field. Remove the stale doc line to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwt.h` around lines 40 - 41, Remove the stale "token_type: Expected typ header value (e.g., \"JWT\")" line from the Validation Options doc comment in src/ngx_oidc_jwt.h (the comment block that lists clock_skew_tolerance) since there is no corresponding struct field; update the comment to only list existing fields (e.g., clock_skew_tolerance) and do not add a new struct member unless you intend to implement token_type validation.src/ngx_oidc_provider.c-13-36 (1)
13-36:⚠️ Potential issue | 🟡 MinorMissing NULL check on
provider_nameparameter.
ngx_oidc_provider_by_nameis a public API (declared inngx_oidc_provider.h) but dereferencesprovider_nameat line 27 without a NULL guard. While current callers validate before calling, a direct invocation withNULLwould crash.🛡️ Proposed fix
ngx_http_oidc_provider_t * ngx_oidc_provider_by_name(ngx_http_request_t *r, ngx_str_t *provider_name) { ngx_http_oidc_main_conf_t *omcf; ngx_http_oidc_provider_t *provider; ngx_uint_t i; + if (r == NULL || provider_name == NULL) { + return NULL; + } + omcf = ngx_http_get_module_main_conf(r, ngx_http_oidc_module);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_provider.c` around lines 13 - 36, ngx_oidc_provider_by_name currently dereferences the provider_name pointer (ngx_str_t *provider_name) without a NULL check; add an early NULL guard at the start of ngx_oidc_provider_by_name to return NULL if provider_name is NULL (before calling ngx_http_get_module_main_conf or before comparing lengths), ensuring you do not access provider_name->len or provider_name->data when provider_name is NULL; keep the rest of the function unchanged so it still retrieves omcf via ngx_http_get_module_main_conf and iterates omcf->providers to match provider[i].name.src/ngx_oidc_jwt.c-536-553 (1)
536-553:⚠️ Potential issue | 🟡 Minor
nbfunderflow guard incorrectly rejects tokens withnbf < clock_skew.When
claims->not_before < params->clock_skew(e.g.,nbf=100, skew=300), the code rejects the token as expired. However, such a token has already passed its not-before time — it should be accepted. The guard prevents signed integer underflow (UB in C), which is correct, but the error path should accept the token (sincenowis certainly >=nbf - skewwhennbfis that small), not reject it.In practice this only affects contrived/test timestamps near the epoch, so the real-world impact is minimal.
🛡️ Proposed fix
/* Check not_before */ if (claims->not_before != 0) { - /* Check for underflow before subtraction */ - if (claims->not_before < params->clock_skew) { - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "oidc_jwt: not_before time underflow " - "(nbf=%T, skew=%T)", - claims->not_before, params->clock_skew); - return JWT_ERR_TOKEN_EXPIRED; - } - - if (now < claims->not_before - params->clock_skew) { + /* Rewrite to avoid underflow: now + skew < nbf */ + if (now + params->clock_skew < claims->not_before) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "oidc_jwt: token not yet valid " "(nbf=%T, now=%T, skew=%T)", claims->not_before, now, params->clock_skew); return JWT_ERR_TOKEN_EXPIRED; } }Note: this mirrors the overflow guard already present for
expat line 496, which you'd also want fornow + clock_skewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwt.c` around lines 536 - 553, The current underflow guard for claims->not_before incorrectly rejects tokens when claims->not_before < params->clock_skew; instead handle the potential underflow by computing an adjusted not-before boundary and only reject if now is before that boundary. Change the logic in the not-before check to: if claims->not_before < params->clock_skew set adjusted_nbf = 0 (or UINT64/0-equivalent) else adjusted_nbf = claims->not_before - params->clock_skew, then if now < adjusted_nbf log the "token not yet valid" message and return JWT_ERR_TOKEN_EXPIRED; mirror the safe overflow-handling pattern used for exp (the code near the existing exp overflow guard).src/ngx_oidc_session_store_memory.h-74-84 (1)
74-84:⚠️ Potential issue | 🟡 MinorDoc for
deleteomitsNGX_DECLINEDreturn value.The implementation returns
NGX_DECLINEDwhen the key is not found (consistent withget), but the doc on Line 81 only listsNGX_OK/NGX_ERROR.📝 Proposed fix
- * `@return` NGX_OK on success, NGX_ERROR on failure + * `@return` NGX_OK on success, NGX_DECLINED if not found, NGX_ERROR on failure🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session_store_memory.h` around lines 74 - 84, Update the documentation comment for ngx_oidc_session_store_memory_delete to include NGX_DECLINED as a possible return value (used when the key is not found, matching ngx_oidc_session_store_memory_get), so state that the function returns NGX_OK on success, NGX_DECLINED when the key does not exist, and NGX_ERROR on failure; ensure the description near the `@return` block mentions NGX_DECLINED explicitly.
🧹 Nitpick comments (20)
src/ngx_oidc_json.h (1)
211-233: Inline type-check helpers returnngx_int_t(0 or 1), but callers may expect a proper boolean.
ngx_oidc_json_is_string,ngx_oidc_json_is_integer, etc. return the result of an enum comparison — which is0or1. Usingngx_int_tas the return type (intptr_tunder nginx) is technically fine but slightly misleading; the nginx convention for boolean-like predicates isngx_uint_tor simplyint. This is a minor readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_json.h` around lines 211 - 233, The helpers ngx_oidc_json_is_string, ngx_oidc_json_is_integer, ngx_oidc_json_is_boolean, and ngx_oidc_json_is_array return the result of an enum comparison but are typed as ngx_int_t; change their return type to a boolean-style type (prefer ngx_uint_t or int per nginx conventions) to make intent clearer and match predicate usage—update the function signatures of ngx_oidc_json_is_string, ngx_oidc_json_is_integer, ngx_oidc_json_is_boolean, and ngx_oidc_json_is_array accordingly while leaving their bodies (the enum comparisons) unchanged.src/ngx_oidc_handler_logout.c (2)
40-47: Redundantu_char *encoded_client_id/encoded_post_logout_uritop-level declarations.These raw pointer aliases (Lines 46–47) are only ever assigned from
encoded_client_id_str.dataandencoded_post_logout_uri_str.datarespectively (Lines 234 and 244) and immediately used to build the URL. Keeping both thengx_str_tstructs and theu_char *aliases adds visual noise without benefit — just useencoded_client_id_str.datadirectly inngx_cpymem.♻️ Suggested cleanup
- u_char *p, *encoded_client_id, *encoded_post_logout_uri; + u_char *p; u_char *encoded_id_token = NULL; // ... - encoded_client_id = encoded_client_id_str.data; - size_t encoded_client_id_len = encoded_client_id_str.len; // ... - encoded_post_logout_uri = encoded_post_logout_uri_str.data; - size_t encoded_post_logout_uri_len = encoded_post_logout_uri_str.len; // ... - p = ngx_cpymem(p, encoded_client_id, encoded_client_id_len); + p = ngx_cpymem(p, encoded_client_id_str.data, encoded_client_id_str.len); // ... - p = ngx_cpymem(p, encoded_post_logout_uri, encoded_post_logout_uri_len); + p = ngx_cpymem(p, encoded_post_logout_uri_str.data, + encoded_post_logout_uri_str.len);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_logout.c` around lines 40 - 47, Remove the redundant top-level u_char pointers encoded_client_id and encoded_post_logout_uri: they are only assigned from encoded_client_id_str.data and encoded_post_logout_uri_str.data and then passed into ngx_cpymem, so delete the declarations for encoded_client_id and encoded_post_logout_uri and update the ngx_cpymem calls to use encoded_client_id_str.data and encoded_post_logout_uri_str.data directly; keep the ngx_str_t variables encoded_client_id_str and encoded_post_logout_uri_str intact and ensure all references to the removed pointers are replaced accordingly.
119-142: C99 mid-block variable declarations deviate from nginx coding style.Nginx traditionally requires all variables to be declared at the beginning of a block, following C89/C90 conventions. Lines 120, 142, 217, 226, 235, 245, 254, and 255 all declare variables after executable statements. While modern C compilers and nginx 1.18+ handle this without issue, it diverges from the traditional nginx style convention.
Consider moving these declarations to the top of the enclosing block (lines 108–340) to maintain consistency with nginx coding practices, though this module does not currently enforce this style requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_logout.c` around lines 119 - 142, Move all variable declarations in the oidc_handler_logout function to the top of its enclosing block so they precede any executable statements; specifically relocate the mid-block declarations such as ngx_http_oidc_ctx_t *ctx, ngx_str_t metadata, ngx_str_t *end_session, ngx_str_t full_logout_url and the other variables declared later in the function to the start of the function/block (before the first statement that uses ngx_http_get_module_ctx or any other logic). Update any initialization expressions to be assignments after the declarations so behavior is unchanged. Ensure you only change declaration positions (not types or initialization logic) and keep references like ngx_http_get_module_ctx, ngx_oidc_metadata_get_end_session_endpoint and subsequent uses intact.src/ngx_oidc_hash.h (1)
61-71:const void *return type loses type safety forngx_oidc_hash_get_md.Callers must silently cast to
const EVP_MD *before use, which bypasses compiler type checking. Since all callers are within the same module and already include<openssl/evp.h>(as seen insrc/ngx_oidc_hash.c), returning the proper type directly would be safer and self-documenting.♻️ Suggested fix
In
src/ngx_oidc_hash.h, add the forward declaration and use the real type:+#include <openssl/evp.h> -const void * ngx_oidc_hash_get_md(const char *algorithm); +const EVP_MD *ngx_oidc_hash_get_md(const char *algorithm);And in
src/ngx_oidc_hash.c:-const void * +const EVP_MD * ngx_oidc_hash_get_md(const char *algorithm)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_hash.h` around lines 61 - 71, Change the prototype of ngx_oidc_hash_get_md from returning const void * to returning const EVP_MD * and add a forward declaration or include for EVP_MD in the header so the real OpenSSL type is visible (e.g., forward-declare typedef struct evp_md_st EVP_MD; or include <openssl/evp.h>), then update the implementation signature in ngx_oidc_hash.c to match const EVP_MD * ngx_oidc_hash_get_md(const char *algorithm) and remove any casts at call sites so callers use the proper const EVP_MD * directly..uncrustify.cfg (1)
175-180: Duplicatealign_var_def_star_stylesetting.
align_var_def_star_style = 2is already set at Line 85. The second assignment here is redundant. Since the values match, there's no functional impact, but it can cause confusion if they ever drift.🔧 Suggested cleanup
# Variable definitions (no alignment - OIDC module has diverse type name lengths) align_var_def_span = 0 -align_var_def_star_style = 2 # Pointer alignment (star attached to name) align_var_def_amp_style = 2 # Reference alignment🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.uncrustify.cfg around lines 175 - 180, Remove the duplicate assignment of align_var_def_star_style in the later variable-definition block: locate the second "align_var_def_star_style = 2" (the one next to align_var_def_amp_style) and delete it so the setting exists only once (the original at the earlier declaration). Keep the surrounding comments and other settings (e.g., align_var_def_span, align_var_def_amp_style) unchanged.config (1)
5-5:ngx_module_depsis empty — header changes won't trigger recompilation.The
ngx_module_depsvariable should list the module's header files so nginx's build system can track dependencies for incremental builds. Without this, modifying any.hfile (e.g.,ngx_http_oidc_module.h) won't trigger recompilation of the affected.cfiles, leading to stale object files.Proposed fix
-ngx_module_deps= +ngx_module_deps="\ + $ngx_addon_dir/src/ngx_http_oidc_module.h \ + $ngx_addon_dir/src/ngx_oidc_handler_authenticate.h \ + $ngx_addon_dir/src/ngx_oidc_handler_callback.h \ + $ngx_addon_dir/src/ngx_oidc_handler_logout.h \ + $ngx_addon_dir/src/ngx_oidc_handler_status.h \ + $ngx_addon_dir/src/ngx_oidc_hash.h \ + $ngx_addon_dir/src/ngx_oidc_http.h \ + $ngx_addon_dir/src/ngx_oidc_json.h \ + $ngx_addon_dir/src/ngx_oidc_jwks.h \ + $ngx_addon_dir/src/ngx_oidc_jwt.h \ + $ngx_addon_dir/src/ngx_oidc_metadata.h \ + $ngx_addon_dir/src/ngx_oidc_provider.h \ + $ngx_addon_dir/src/ngx_oidc_random.h \ + $ngx_addon_dir/src/ngx_oidc_session.h \ + $ngx_addon_dir/src/ngx_oidc_session_store.h \ + $ngx_addon_dir/src/ngx_oidc_session_store_memory.h \ + $ngx_addon_dir/src/ngx_oidc_session_store_redis.h \ + $ngx_addon_dir/src/ngx_oidc_url.h \ +"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config` at line 5, The ngx_module_deps variable is empty so header changes won't trigger recompilation; update the ngx_module_deps definition to list the module's public/private header files (e.g., ngx_http_oidc_module.h and any other .h files used by the module) so the build system tracks dependencies and will recompile affected .c files; edit the ngx_module_deps entry to include those header filenames separated appropriately to match the existing config format.src/ngx_oidc_session.c (1)
183-292: Partial rotation failure leaves inconsistent state.If rotation succeeds in copying some tokens to the new session but fails midway (e.g., access_token set fails at Line 221), the function returns
NGX_ERRORleaving the old session intact and partial data in the new session. The old session data is only deleted at Lines 267–285 after all copies succeed. However, the partially-written new session keys are never cleaned up on error.The acknowledged comment about non-atomicity (Lines 171–181) is appreciated, but consider at least cleaning up the new session keys on partial failure to avoid dangling session data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session.c` around lines 183 - 292, The ngx_oidc_session_rotate function can leave partial data in the new session if a later copy fails; update ngx_oidc_session_rotate to track which keys were successfully set and, on any intermediate NGX_ERROR return, delete those keys from new_session_id using ngx_oidc_session_delete to roll back; specifically record successful sets for NGX_OIDC_SESSION_KEY_ID_TOKEN, NGX_OIDC_SESSION_KEY_ACCESS_TOKEN, NGX_OIDC_SESSION_KEY_REFRESH_TOKEN, and NGX_OIDC_SESSION_KEY_USERINFO (the keys passed to ngx_oidc_session_set), and call ngx_oidc_session_delete(r, store, new_session_id, <key>) for each recorded key before returning NGX_ERROR from ngx_oidc_session_rotate.src/ngx_oidc_handler_authenticate.c (2)
91-103:redirect_uriis validated before the empty check — could produce a misleading error.If
redirect_urievaluates to an empty string,ngx_oidc_url_validateis called first. Depending on that function's behavior with empty input, you might get a "validation failed" error when the actual issue is "no redirect_uri available". Swap the order so the empty check comes first.Proposed fix
- /* Validate redirect_uri parameter format */ - if (ngx_oidc_url_validate(r, &redirect_uri) != NGX_OK) { - ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, - "oidc_handler_authenticate: redirect_uri parameter " - "validation failed"); - return NULL; - } if (redirect_uri.len == 0) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "oidc_handler_authenticate: no redirect_uri available " "from provider"); return NULL; } + + /* Validate redirect_uri parameter format */ + if (ngx_oidc_url_validate(r, &redirect_uri) != NGX_OK) { + ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, + "oidc_handler_authenticate: redirect_uri parameter " + "validation failed"); + return NULL; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_authenticate.c` around lines 91 - 103, The current code calls ngx_oidc_url_validate(r, &redirect_uri) before checking for an empty redirect_uri, which can produce a misleading "validation failed" error when the real problem is a missing redirect URI; modify the logic in ngx_oidc_handler_authenticate so you first check if redirect_uri.len == 0 and log the "no redirect_uri available from provider" error (using ngx_log_error) and return NULL, and only then call ngx_oidc_url_validate(r, &redirect_uri) and handle its failure with the "redirect_uri parameter validation failed" log path.
371-375: Redundant double-initialization ofcode_verifier.
code_verifieris zeroed viangx_memzeroat line 375, then null'd again viangx_str_nullat line 450. The second initialization is unnecessary since the only path between them either returns early or doesn't modifycode_verifier.Also applies to: 449-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_authenticate.c` around lines 371 - 375, The variable code_verifier is being initialized twice (once with ngx_memzero(&code_verifier, ...) and again with ngx_str_null(&code_verifier)); remove the redundant early zeroing so only one clear initialization remains. Specifically, delete the ngx_memzero(&code_verifier, sizeof(ngx_str_t)) call and keep the later ngx_str_null(&code_verifier) usage in the authenticate handler to ensure a single clear initialization of code_verifier.src/ngx_oidc_handler_status.h (1)
13-20: Docstring@returnis incomplete — implementation also returnsNGX_HTTP_*status codes.Per the implementation in
src/ngx_oidc_handler_status.c, this handler can returnNGX_HTTP_NOT_ALLOWED,NGX_HTTP_INTERNAL_SERVER_ERROR, etc., in addition toNGX_OKandNGX_ERROR. Consider updating the@returnto match, e.g.,@return NGX_OK, NGX_ERROR, or NGX_HTTP_* status code.Proposed fix
- * `@return` NGX_OK or NGX_ERROR + * `@return` NGX_OK, NGX_ERROR, or NGX_HTTP_* status code🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_status.h` around lines 13 - 20, Update the docstring for the function ngx_oidc_handler_status to accurately reflect all possible return values: change the `@return` line in the header comment block to something like "@return NGX_OK, NGX_ERROR, or an NGX_HTTP_* status code (e.g., NGX_HTTP_NOT_ALLOWED, NGX_HTTP_INTERNAL_SERVER_ERROR)" so it matches the implementation in ngx_oidc_handler_status.c and documents that the handler may return NGX_HTTP_* codes in addition to NGX_OK/NGX_ERROR..github/workflows/test.yaml (1)
27-29:apt-get upgradein CI makes builds non-deterministic and slow.Full system upgrades can introduce unexpected package changes and significantly slow down CI runs. Consider removing it unless there's a specific CVE being mitigated.
Proposed fix
sudo apt-get update - sudo apt-get upgrade -y --no-install-recommends sudo apt-get install -y --no-install-recommends \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yaml around lines 27 - 29, The CI workflow run step currently performs a full system upgrade using "apt-get upgrade -y --no-install-recommends" which makes builds non-deterministic and slow; remove that upgrade command from the run block and keep only "sudo apt-get update" (or replace with a targeted package install if a specific CVE or package version is required), ensuring the run section no longer executes a full system upgrade during the test job.src/ngx_oidc_handler_status.c (2)
524-533: Mixed declaration and code — not C89-compatible.Line 527 declares
status_metadata_ctx_t ctxafter thengx_cpymemstatement on line 525, which is valid in C99 but not in C89. nginx core code traditionally targets C89 for maximum portability. Wrap in a block or move the declaration to the top of theifbody.♻️ Proposed fix
/* Format Metadata details */ if (metadata_entries > 0) { + status_metadata_ctx_t ctx; + b->last = ngx_cpymem(b->last, "Metadata:\n", sizeof("Metadata:\n") - 1); - status_metadata_ctx_t ctx; ctx.buf = b; ctx.size = NULL; ctx.count = NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_status.c` around lines 524 - 533, The declaration of status_metadata_ctx_t ctx appears after code (ngx_cpymem) which violates C89; move the declaration to the start of the if block (before the ngx_cpymem call) or wrap the existing code and declaration inside its own inner block so ctx is declared before any statements. Specifically adjust the if (metadata_entries > 0) block that uses ngx_cpymem, status_metadata_ctx_t ctx, and ngx_oidc_metadata_iterate with status_metadata_format_callback so the ctx variable is declared prior to any executable statements.
128-143: Silent data truncation instatus_write_indentedwhen buffer is full.When the bounds check at line 130 fails, the line is silently dropped. For a diagnostic endpoint this may cause confusing output. Consider at minimum logging a debug message, or pre-computing a tighter buffer size.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_status.c` around lines 128 - 143, status_write_indented silently drops lines when the check "b->last + indent + (p - line_start) + 1 <= b->end" fails; update status_write_indented to handle a full buffer explicitly by either logging a debug/error via ngx_log_debug*/ngx_log_error with context (function name and line length) or by writing a truncated version (e.g., write as much as fits then append "...\n") instead of silently skipping; reference the buffer pointers (b->last, b->end), the indent variable, and the line_start/p pointers to compute the writable length and perform the safe partial copy or emit the log before advancing line_start.src/ngx_oidc_session_store_redis.h (1)
12-12: Unnecessaryhiredis/hiredis.hinclude in public header.No hiredis types appear in any of the function declarations — they all use nginx types (
ngx_int_t,ngx_str_t,ngx_http_request_t, etc.) and the forward-declaredngx_oidc_session_store_t. Moving this include tongx_oidc_session_store_redis.creduces coupling for consumers.♻️ Proposed fix
`#include` <ngx_config.h> `#include` <ngx_core.h> `#include` <ngx_http.h> -#include <hiredis/hiredis.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session_store_redis.h` at line 12, The public header ngx_oidc_session_store_redis.h should not include <hiredis/hiredis.h> since no hiredis types appear in the function declarations; remove the `#include` <hiredis/hiredis.h> from ngx_oidc_session_store_redis.h and instead add that include to ngx_oidc_session_store_redis.c where hiredis types are actually used (e.g., in functions that implement redis interactions for ngx_oidc_session_store_t, and any helper functions referencing hiredis types). Ensure the header still forward-declares ngx_oidc_session_store_t and exposes functions using only nginx types (ngx_int_t, ngx_str_t, ngx_http_request_t) so consumers are not coupled to hiredis.src/ngx_oidc_jwt.h (1)
12-19: Unnecessary OpenSSL headers in public API header.None of the types from these OpenSSL headers (
EVP_PKEY,RSA,EC_KEY,BIGNUM, etc.) appear in the public API surface of this header. Moving them tongx_oidc_jwt.cwould reduce coupling and compile-time dependencies for consumers.♻️ Proposed fix
`#include` <ngx_config.h> `#include` <ngx_core.h> `#include` <ngx_http.h> -#include <openssl/evp.h> -#include <openssl/rsa.h> -#include <openssl/ec.h> -#include <openssl/bn.h> -#include <openssl/param_build.h> -#include <openssl/core_names.h> -#include <openssl/err.h> -#include <openssl/obj_mac.h> `#include` "ngx_oidc_json.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwt.h` around lines 12 - 19, The public header ngx_oidc_jwt.h currently includes unnecessary OpenSSL headers (EVP, RSA, EC, BIGNUM, param_build, core_names, err, obj_mac) that are not part of the public API; remove these `#include` lines from ngx_oidc_jwt.h and move them into ngx_oidc_jwt.c so only the implementation sees EVP_PKEY/RSA/EC_KEY/BIGNUM types and OpenSSL symbols, keeping the header limited to forward declarations and its public declarations (leave any public typedefs/functions as-is but ensure implementations in functions like those that manipulate JWT/OpenID keys reference the OpenSSL headers inside ngx_oidc_jwt.c).src/ngx_oidc_jwt.c (1)
567-610: Use-after-free risk:algorithmpointer checked afterjson_decref.At line 604,
algorithm(a pointer intoheader_json's memory) is checked for NULL afterngx_oidc_json_free(header_json)at line 599. The NULL check itself is safe, but the pointer is dangling — any future code that reads through it would be UaF. Consider setting a boolean flag instead.Additionally, re-parsing the JWT header here is redundant since
jwt_verify_signaturealready parsed it. Passing the algorithm through thejwt_claims_tstruct (it's already available asclaims->at_hash's sibling in the payload) or through the verification context would avoid the extra work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwt.c` around lines 567 - 610, The code re-parses the JWT header to set a pointer variable algorithm into header_json's memory then frees header_json with ngx_oidc_json_free(header_json) and later checks/uses algorithm, creating a use-after-free risk; fix by not retaining a pointer into freed JSON — either extract/copy the alg value into a local ngx_str_t or boolean flag before freeing (e.g. set has_alg = true and copy algorithm bytes into a stack/pooled ngx_str_t) and use that copy for ngx_oidc_jwt_validate_at_hash, or better yet avoid re-parsing entirely by plumbing the parsed alg out of the initial jwt parsing/verification (e.g. add algorithm to claims or verification context produced by jwt_verify_signature and use claims->alg) and call ngx_oidc_jwt_validate_at_hash with that safe value instead of accessing header_json after ngx_oidc_json_free; update references: ngx_oidc_jwt_decode_header, ngx_oidc_json_parse, ngx_oidc_json_object_get, ngx_oidc_json_string, ngx_oidc_json_free, ngx_oidc_jwt_validate_at_hash, and the claims struct (claims->at_hash / add claims->alg) accordingly.src/ngx_oidc_jwks.c (2)
979-1034:jwks_shm_lookupreturns with lock held on success — asymmetric lock contract.The function holds the mutex on return when a node is found (Line 1022) but releases it when not found (Line 1032). This is the correct nginx pattern used in the codebase, and callers (
ngx_oidc_jwks_get) handle both paths properly. However, a brief doc comment on the function noting this contract would help maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwks.c` around lines 979 - 1034, The jwks_shm_lookup function currently returns with the shared-memory mutex still held when a node is found but unlocks the mutex when not found; add a concise doc comment atop jwks_shm_lookup describing this lock contract (i.e., on success the caller receives a jwks_shm_node_t* with the shm->shpool->mutex still locked and is responsible for unlocking, while on failure the function has already unlocked the mutex and returns NULL) and mention that callers such as ngx_oidc_jwks_get must handle both cases accordingly.
906-916: Hard-coded 1-hour TTL in multiple locations.The TTL
3600appears at Lines 906, 916, and 1265. Consider extracting this to a named constant (e.g.,#define NGX_OIDC_JWKS_DEFAULT_TTL 3600) so it can be found and changed in one place. Ideally, this would be configurable via the nginx conf, but a constant is a good first step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwks.c` around lines 906 - 916, Replace the repeated hard-coded TTL literal (3600) with a named constant: add a macro like NGX_OIDC_JWKS_DEFAULT_TTL (value 3600) and use it wherever TTL is set (e.g., assignments to key->expires_at, the call to ngx_oidc_jwks_cache_node_set_expires_at, and any other occurrences such as the place at line ~1265); update the code paths that set fetched/expires times (references: key->expires_at, ngx_oidc_jwks_cache_node_set_fetched_at, ngx_oidc_jwks_cache_node_set_expires_at) to use NGX_OIDC_JWKS_DEFAULT_TTL so the TTL value is centralized (optionally later expose it via nginx config).src/ngx_oidc_session_store_memory.c (2)
522-524: Unnecessary queue remove/reinsert inget— expiration is unchanged.
getdoes not modifyocn->expires, so removing and reinserting the node in the expire queue places it back at the same position. This is wasted work under the shared-memory mutex on every read.If this was intended for future LRU-style touch semantics, consider adding a comment. Otherwise, remove it.
♻️ Proposed fix (remove no-op reinsert)
ngx_memcpy(value->data, ocn->data.data, value->len); - /* Reinsert into expire queue at correct sorted position */ - ngx_queue_remove(&ocn->queue); - mem_queue_insert_expire(&octx->expire_queue, ocn); - ngx_shmtx_unlock(&octx->shpool->mutex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session_store_memory.c` around lines 522 - 524, The get path currently calls ngx_queue_remove(&ocn->queue) and mem_queue_insert_expire(&octx->expire_queue, ocn) even though ocn->expires is not changed, causing an unnecessary remove/reinsert under the shared-memory mutex; remove those two calls from the get implementation (the calls around ocn and octx in the get function) so reads do not touch the expire queue, or if you intended future LRU touch behavior add a clear comment in the get function referencing ocn, octx, expire_queue, ngx_queue_remove and mem_queue_insert_expire to justify keeping them.
598-641:shm_zone_initdiscards previous zone data on reload.The
dataparameter (old zone data) is explicitly ignored, so all in-flight sessions are lost on every nginx configuration reload. This is an acceptable trade-off for simplicity, but worth noting for operators. If session persistence across reloads matters, the old rbtree data could be reused (similar to the JWKS zone init pattern inngx_oidc_jwks_init_zone).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session_store_memory.c` around lines 598 - 641, The shm zone init currently ignores the incoming data parameter and always allocates a fresh shm context, causing prior zone state to be discarded on reload; update ngx_oidc_session_store_memory_shm_zone_init to check the data parameter (old zone data) and, when non-NULL, reuse the existing shm context instead of clobbering it: cast data to the existing shm context type (the same struct allocated as octx), preserve/reuse its rbtree and expire_queue (do not reinitialize them), update shpool and max_size from the new store if needed, and set shm_zone->data to the existing context; follow the same approach used by ngx_oidc_jwks_init_zone and ensure ngx_oidc_session_store_memory_setup_zones still initializes shm_zone->data to the store pointer when creating new zones.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
src/ngx_oidc_handler_status.h (1)
13-19: Documentation return value is incomplete.The docstring states
@return NGX_OK or NGX_ERROR, but the implementation insrc/ngx_oidc_handler_status.creturns variousNGX_HTTP_*status codes (NGX_HTTP_OK,NGX_HTTP_NOT_ALLOWED,NGX_HTTP_INTERNAL_SERVER_ERROR).📝 Suggested documentation fix
/** * Handles status endpoint requests for JWKS and Metadata cache statistics * * `@param`[in] r HTTP request * - * `@return` NGX_OK or NGX_ERROR + * `@return` NGX_HTTP_* status code */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_handler_status.h` around lines 13 - 19, Update the `@return` doc in the ngx_oidc_handler_status.h comment to match the actual return values used by the implementation (see ngx_oidc_handler_status.c); replace the generic "NGX_OK or NGX_ERROR" with the specific HTTP status codes returned by the handler (e.g., NGX_HTTP_OK, NGX_HTTP_NOT_ALLOWED, NGX_HTTP_INTERNAL_SERVER_ERROR) and a short note that the function returns an ngx_int_t HTTP status code per ngx_http handler conventions.src/ngx_oidc_random.h (1)
13-81: Document output buffer ownership/lifetime in this public API.Right now the docs describe what gets generated, but not where the output is allocated / who owns it (e.g., request pool vs caller-owned). Since these are public helpers, callers need that contract spelled out.
Proposed doc tweak
`#include` <ngx_core.h> `#include` <ngx_http.h> +/* + * Memory ownership: + * - Output buffers are allocated by these helpers (typically from r->pool). + * - Callers should treat returned ngx_str_t as binary-safe via .len, and must + * not attempt to free the backing storage directly. + */ + /** * Generate random state parameter for OAuth 2.0 CSRF protection *🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_random.h` around lines 13 - 81, Update the function docs to specify buffer ownership/lifetime: for ngx_oidc_random_state, ngx_oidc_random_nonce, ngx_oidc_random_code_verifier, ngx_oidc_random_code_challenge, and ngx_oidc_random_session_id state that on success (NGX_OK) the returned ngx_str_t->data is allocated from the request pool (r->pool), is NUL-terminated if applicable, must not be freed by the caller, and remains valid until the request pool is destroyed; also state that on failure the output is undefined/unchanged. Add this ownership contract to each function comment in ngx_oidc_random.h.Dockerfile (1)
53-63: Consider running the final image as non-root.Trivy flags the image running as root. If feasible, create a non-root user and run on an unprivileged port (or add only the minimal capabilities required).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 53 - 63, The image is currently running as root; update the Dockerfile (the stage starting with "FROM nginx AS module") to create and switch to a non-root user and ensure nginx runs on an unprivileged port or with minimal capabilities: add a non-root user/group, chown the nginx config and any runtime directories modified by the RUN block (including /etc/nginx and any cache or log dirs touched during the RUN --mount block), update nginx.conf (the file referenced by the sed line that inserts load_module "/usr/lib/nginx/modules/ngx_http_oidc_module.so") to listen on an unprivileged port (e.g., 8080) or adjust the container runtime to drop privileges, and add a USER <nonroot> directive in the final image stage so the container does not run as root. Ensure file permissions are set so the non-root user can read the module and write to logs/temp.src/ngx_oidc_jwt.c (1)
1338-1345: Consider using explicit algorithm checks for hash_half_len determination.Similar to the concern in
ngx_oidc_hash_get_md, thengx_strstrcalls here could match unintended algorithm strings. While the impact is limited (only affects hash truncation length), using explicit algorithm comparisons would be more robust and consistent with secure coding practices.♻️ Suggested improvement
- if (ngx_strstr(algorithm, "256")) { - hash_half_len = 16; /* SHA-256 produces 32 bytes, take left 16 */ - } else if (ngx_strstr(algorithm, "384")) { - hash_half_len = 24; /* SHA-384 produces 48 bytes, take left 24 */ - } else { - /* 512 or EdDSA */ - hash_half_len = 32; /* SHA-512 produces 64 bytes, take left 32 */ - } + if (ngx_strcmp(algorithm, "RS256") == 0 + || ngx_strcmp(algorithm, "ES256") == 0 + || ngx_strcmp(algorithm, "PS256") == 0 + || ngx_strcmp(algorithm, "HS256") == 0) + { + hash_half_len = 16; /* SHA-256 produces 32 bytes, take left 16 */ + } else if (ngx_strcmp(algorithm, "RS384") == 0 + || ngx_strcmp(algorithm, "ES384") == 0 + || ngx_strcmp(algorithm, "PS384") == 0 + || ngx_strcmp(algorithm, "HS384") == 0) + { + hash_half_len = 24; /* SHA-384 produces 48 bytes, take left 24 */ + } else { + /* RS512, ES512, PS512, HS512, EdDSA */ + hash_half_len = 32; /* SHA-512 produces 64 bytes, take left 32 */ + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_jwt.c` around lines 1338 - 1345, The code uses ngx_strstr(algorithm, "...") which can match unintended strings; update the logic in ngx_oidc_jwt.c where hash_half_len is set (variable algorithm) to use explicit comparisons against known JWT alg values (e.g. "HS256","RS256","ES256","PS256" -> hash_half_len=16; "HS384","RS384","ES384","PS384" -> 24; and "HS512","RS512","ES512","PS512","EdDSA" -> 32) instead of substring searches; implement these checks with exact string comparisons (ngx_strcmp/ngx_strncmp or equivalent) to ensure only the intended algorithms set each truncation length.src/ngx_oidc_session_store_memory.c (1)
522-524: Remove unnecessary queue reordering in get operation.The code removes the node from the expiration queue and reinserts it, but since the expiration time (
ocn->expires) hasn't changed, the node will be reinserted at the same position. This is unnecessary work that holds the lock longer than needed.If the intent is LRU behavior (moving accessed items to prevent eviction), the expiration time should be updated. Otherwise, these lines should be removed.
🧹 Proposed fix (remove unnecessary reordering)
ngx_memcpy(value->data, ocn->data.data, value->len); - /* Reinsert into expire queue at correct sorted position */ - ngx_queue_remove(&ocn->queue); - mem_queue_insert_expire(&octx->expire_queue, ocn); - ngx_shmtx_unlock(&octx->shpool->mutex); return NGX_OK;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session_store_memory.c` around lines 522 - 524, The get path is doing unnecessary queue reordering: remove the calls to ngx_queue_remove(&ocn->queue) and mem_queue_insert_expire(&octx->expire_queue, ocn) in the get operation since ocn->expires is not modified and reinsertion will place the node in the same spot, holding the lock longer; if you intended LRU semantics instead, update ocn->expires before reinserting and then call ngx_queue_remove + mem_queue_insert_expire, otherwise delete those two lines and any comments referencing reordering to avoid needless work.src/ngx_oidc_session.c (1)
171-292: Partial rotation failure leaves duplicate data in new session.When rotation fails mid-way (e.g.,
access_tokenmove fails afterid_tokensucceeds), the function returnsNGX_ERRORwithout cleaning up the partially-moved data from the new session. This leavesid_tokenin both sessions.While the comment acknowledges non-atomicity, consider cleaning up the new session on failure to avoid data duplication:
♻️ Proposed rollback on failure
Add cleanup of the new session when rotation fails:
/* Move Access Token */ rc = ngx_oidc_session_get(r, store, old_session_id, NGX_OIDC_SESSION_KEY_ACCESS_TOKEN, &token_data); if (rc == NGX_OK) { rc = ngx_oidc_session_set(r, store, new_session_id, NGX_OIDC_SESSION_KEY_ACCESS_TOKEN, &token_data, expires); if (rc != NGX_OK) { ngx_log_error(NGX_LOG_ERR, r->connection->log, 0, "oidc_session: failed to move access_token " "to new session, aborting rotation"); + /* Clean up partially moved data */ + ngx_oidc_session_delete(r, store, new_session_id, + NGX_OIDC_SESSION_KEY_ID_TOKEN); return NGX_ERROR; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ngx_oidc_session.c` around lines 171 - 292, ngx_oidc_session_rotate can leave partially-moved data in the new session when a later ngx_oidc_session_set fails; modify ngx_oidc_session_rotate so that on any failure after a successful ngx_oidc_session_set it calls ngx_oidc_session_delete for the same keys on new_session_id to roll back partial moves (delete NGX_OIDC_SESSION_KEY_ID_TOKEN, NGX_OIDC_SESSION_KEY_ACCESS_TOKEN, NGX_OIDC_SESSION_KEY_REFRESH_TOKEN, NGX_OIDC_SESSION_KEY_USERINFO as applicable), keep error logging via r->connection->log, and return NGX_ERROR; implement the cleanup in each failure branch inside ngx_oidc_session_rotate (use the same ngx_oidc_session_delete helper used for old_session_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 92-95: Replace the hex OIDC_CLIENT_SECRET value with a non-secret
test string across all test configs: update the environment variable in the CI
workflow (the OIDC_CLIENT_SECRET entry in the .github/workflows/test.yaml diff)
and every test provider config that contains b028e7a42bbb072acf09ed342e760627
(files referenced: t/conf/test-provider.conf, t/conf/test-provider-redis.conf,
t/conf/test-provider-logout.conf, and all variants of
t/conf/test-provider-alg.conf) to a clearly non-secret value like
test-secret-value so Gitleaks won’t flag it and all tests remain consistent.
- Around line 14-24: The GHCR login fails because the job token lacks package
scope; update the job-level permissions for the test job (the permissions map
under job name "test") to include packages: read (e.g., add packages: read
alongside contents: read) so that the docker/login-action that authenticates to
ghcr.io can use the token; alternatively set packages: read in the top-level
permissions block if you want it globally.
In `@Dockerfile`:
- Around line 175-180: The Dockerfile currently defines
NGINX_GOOGLE_CLIENT_SECRET (and other Google auth ENV vars) directly via the ENV
instruction, which bakes secrets into the image; remove the secret from the
Dockerfile and switch to runtime secret injection instead (e.g., use
Docker/Kubernetes secrets, docker-compose env_file or runtime -e, or inject via
entrypoint reading from a mounted secret file) so NGINX_GOOGLE_CLIENT_SECRET is
not present in the image history; update any references to
NGINX_GOOGLE_CLIENT_SECRET and documentation to show the secret must be provided
at deploy/run time rather than in the Dockerfile.
In `@docs/EXAMPLES.md`:
- Around line 1-618: The file docs/EXAMPLES.md is using ATX-style level-1/2
headings (e.g., "## Table of Contents", "## 1. Quick Start", "### 1.1 Minimal
Configuration") which triggers markdownlint MD003 because the repo expects
setext-style for top-level headings; fix by converting the affected ATX headings
to setext style for level-1/2 (replace "## Title" with "Title" followed by a
line of === or --- as appropriate, and similarly convert level-2 where required)
for sections such as "Table of Contents", "1. Quick Start", "1.1 Minimal
Configuration", "2. Configuration Examples", etc., or alternatively update the
markdownlint config to disable/override MD003 if ATX headings are preferred—pick
one approach and apply it consistently across EXAMPLES.md.
In `@README.md`:
- Line 1438: Change the compound modifier "session save" to the hyphenated form
"session-save" where it appears as a modifier in the README (replace occurrences
of the phrase "session save" used as an adjective with "session-save"); ensure
surrounding sentences retain correct spacing and punctuation and run a quick
spell/grammar check to confirm no other instances remain.
- Around line 818-821: The blockquote contains a blank line that triggers
markdownlint MD028; remove the empty line between the two quote lines or prefix
that blank line with ">" so the blockquote remains continuous—update the lines
containing "> **Warning**: For security reasons, restrict access to the status
endpoint appropriately (e.g., with IP address restrictions)." and ">
**Security**: In production environments, it is recommended to configure access
restrictions using `allow`/`deny` directives." to eliminate the blank quoted
line.
- Around line 4-12: The README uses ATX headings (e.g., "## 1. Overview", "###
1.1 About This Module") which triggers markdownlint MD003; either convert these
ATX headings to setext style for top-level headings (e.g., replace "## 1.
Overview" with "1. Overview" followed by an underline of '=' or '-' as
appropriate and similarly for subsection setext underlines) across the file, or
update the markdownlint config to disable/relax MD003 for this repo (adjust
.markdownlint.json or equivalent) so ATX headings are allowed; pick one approach
and apply it consistently to all headings in README.md.
In `@src/ngx_http_oidc_module.c`:
- Around line 321-399: The callback detection fails when an absolute
redirect_uri contains query or fragment components because uri_to_match
currently includes everything from the third '/' to end-of-string; modify the
extraction logic (where redirect_uri, path_start, uri_to_match are handled) to
stop uri_to_match at the first '?' or '#' after path_start (i.e., set end =
pointer to '?' or '#' if present, otherwise original end) before computing
uri_to_match.len, validate pointer ordering as before, and then compare
uri_to_match against r->uri.
In `@src/ngx_oidc_handler_authenticate.c`:
- Around line 84-103: The code is validating provider->redirect_uri directly
which rejects relative paths; modify the flow in ngx_oidc_handler_authenticate
after ngx_http_complex_value fills redirect_uri to first call
ngx_oidc_url_build_absolute(r, &redirect_uri) to convert relative redirect_uri
values to an absolute URL, check its return value for NGX_OK, then pass the
resulting absolute redirect_uri to ngx_oidc_url_validate; keep the existing
error logging and return NULL on failures (refer to ngx_http_complex_value,
redirect_uri, ngx_oidc_url_build_absolute, and ngx_oidc_url_validate).
In `@src/ngx_oidc_handler_authenticate.h`:
- Around line 9-22: The header declares
ngx_oidc_handler_authenticate(ngx_http_request_t *r, ngx_http_oidc_provider_t
*provider) but does not define or forward-declare ngx_http_oidc_provider_t; add
a declaration to avoid compilation errors by either including the module header
that defines the type (e.g., add `#include` "ngx_http_oidc_module.h") or add a
minimal forward declaration (typedef struct ngx_http_oidc_provider_s
ngx_http_oidc_provider_t;) at the top of this header so the symbol
ngx_http_oidc_provider_t referenced by ngx_oidc_handler_authenticate is known to
callers.
In `@src/ngx_oidc_handler_callback.c`:
- Around line 1406-1518: The ORIGINAL_URI (NGX_OIDC_SESSION_KEY_ORIGINAL_URI) is
deleted when ngx_oidc_session_rotate() removes the old session but it is not
migrated to the new session, so callback_redirect() cannot find the original
URI; update ngx_oidc_session_rotate() to include
NGX_OIDC_SESSION_KEY_ORIGINAL_URI in the set of keys copied from old_session_id
to new_session_id (perform the copy before deleting the old session) so that
callback_redirect() can retrieve the original URI using either session ID
(ensure any helper functions used for copying are invoked for the ORIGINAL_URI
key).
In `@src/ngx_oidc_handler_logout.h`:
- Around line 9-24: The header ngx_oidc_handler_logout.h declares ngx_int_t
ngx_oidc_handler_logout(ngx_http_request_t *r, ngx_http_oidc_provider_t
*provider) but does not include the definition of ngx_http_oidc_provider_t;
update the header to be self-contained by adding `#include`
"ngx_http_oidc_module.h" immediately after the existing NGINX includes (after
`#include` <ngx_http.h>) so the type ngx_http_oidc_provider_t is defined for
callers of ngx_oidc_handler_logout.
---
Duplicate comments:
In @.github/workflows/package.yaml:
- Around line 54-63: The GitHub Action step "Build and push for Google example"
currently tags images as `${{ env.container }}:google`, which pushes to Docker
Hub and causes both architectures to overwrite the same tag; change the `tags`
value in that step (the docker/build-push-action@...) to include the GHCR prefix
and an architecture-specific suffix, e.g. `ghcr.io/${{ env.container }}:${{
matrix.arch }}-google` (or `ghcr.io/${{ env.container }}:${{ matrix.arch
}}.google`) so images are pushed to GHCR and are unique per `${{ matrix.arch
}}`.
- Around line 97-106: The step "Build and push for Google example" produces
invalid image names because the image references use "${CONTAINER}${TAG}-amd64"
and "${CONTAINER}${TAG}-arm64" without a ":" separator; update the docker buildx
imagetools create arguments to use "${CONTAINER}:${TAG}-amd64" and
"${CONTAINER}:${TAG}-arm64" (and keep the target tag "-t
\"${CONTAINER}:${TAG}\"") so CONTAINER and TAG are separated correctly.
In @.github/workflows/test.yaml:
- Around line 64-67: Replace the insecure HTTP nginx download with HTTPS and
make curl fail loudly: update the curl invocation that uses the nginx_version
variable (see nginx_version="$(nginx -v ...)" and the curl -sL -o
"nginx-${nginx_version}.tar.gz" ... line) to use
"https://nginx.org/download/nginx-${nginx_version}.tar.gz" and add the -f flag
to curl (so failures exit non‑zero) while keeping any needed -s/-S/-L options.
- Around line 87-100: Remove the output suppression so CI retains startup and
test stderr: stop using "&>/dev/null" on the docker run that launches the
"oidc-stub" container (or redirect its stdout/stderr to a log file and run
"docker logs oidc-stub" afterwards) and stop redirecting test stderr away in the
"prove -r t 2> /dev/null" command (either let stderr flow to the job log or
redirect to a retained file/artifact and fail the job if non-empty); update the
lines that run the oidc-stub container and the "prove -r t" invocation
accordingly.
In `@Dockerfile`:
- Around line 28-30: The Dockerfile currently downloads nginx using an insecure
HTTP URL constructed with nginx_version (nginx_version=$(nginx -v ...)) via curl
-sL -o nginx-${nginx_version}.tar.gz
http://nginx.org/download/nginx-${nginx_version}.tar.gz; change the download URL
to HTTPS (use https://nginx.org/download/nginx-${nginx_version}.tar.gz) so curl
fetches over TLS, and keep the rest of the pipeline (curl and tar -xf
nginx-${nginx_version}.tar.gz) unchanged.
In `@src/ngx_oidc_handler_logout.c`:
- Around line 175-201: Extract the duplicated send-header/output-filter/finalize
sequence used in ngx_oidc_handler_logout into a single static helper (e.g.,
ngx_oidc_finalize_after_redirect) that accepts ngx_http_request_t *r and the
current ngx_int_t rc or computes rc internally; this helper should call
ngx_http_send_header, check rc for NGX_ERROR or rc > NGX_OK and return rc, call
ngx_http_output_filter and return on NGX_ERROR, check r != r->connection->data
and log the debug message before returning NGX_DONE, otherwise call
ngx_http_finalize_request(r, rc) and return NGX_DONE; replace the duplicated
blocks in ngx_oidc_handler_logout (and the similar block found later) with calls
to this helper to centralize behavior.
- Around line 247-279: The code blindly appends "?client_id=..." to
end_session->data causing malformed URLs when end_session already has a query
string; update the logic that computes url_len and that builds the string
(around symbols end_session, url_len, encoded_client_id_len, ngx_cpymem, p,
logout_url) to detect whether end_session->data contains a '?' (and whether it
already ends with '?' or '&') and choose the proper separator ("?" vs "&" or
empty if already present), adjust the added separator length used in url_len
accordingly, and use that chosen separator when copying into logout_url so the
final URL is syntactically correct.
In `@src/ngx_oidc_handler_status.c`:
- Around line 460-463: The format specifier for the value returned by
ngx_oidc_session_store_memory_stats_get_max_entries (which is size_t) is wrong:
update the ngx_snprintf call that writes into b->last (using b->end - b->last)
to use "%uz" instead of "%ui" so the size_t is printed safely (locate the
ngx_snprintf invocation referencing b->last, b->end and
ngx_oidc_session_store_memory_stats_get_max_entries and change the format
specifier accordingly).
In `@src/ngx_oidc_hash.c`:
- Around line 112-140: ngx_oidc_hash_get_md currently uses substring checks
(ngx_strstr) which can incorrectly match crafted alg names; replace those checks
with explicit comparisons against the approved RFC7518 algorithm identifiers
(e.g. "RS256","ES256","PS256","HS256","ES256K" => EVP_sha256(),
"RS384","ES384","PS384","HS384" => EVP_sha384(), "RS512","ES512","PS512","HS512"
=> EVP_sha512(), and explicitly "EdDSA" => EVP_sha512()). Update the function to
test the algorithm string with exact comparisons (ngx_strcmp or exact-length
ngx_strncmp) for each listed identifier and return NULL for anything not exactly
matched. Ensure comparisons remain case-sensitive and preserve the existing NULL
input guard.
In `@src/ngx_oidc_http.c`:
- Around line 112-145: The code only reads the first ngx_buf_t (sr->out->buf and
single cl->buf) which truncates multi-buffer responses; update the logic that
collects the response body (around the http_response_body_copy calls) to iterate
and concatenate across the full ngx_chain_t/ngx_buf_t chains instead of passing
a single buffer: for sr->out walk its chain (follow sr->out->next or use the
chain head if present), and for upstream use sr->upstream->buffer plus iterate
sr->upstream->out_bufs each cl->buf, feeding each buf segment into the
response-accumulation routine (or extend http_response_body_copy to accept a
chain) until all data (or last_buf) is consumed, preserving order and handling
partial/temporary buffers via ngx_buf_size/pos/last. Ensure checks still handle
NULL sr->upstream and return the same rc semantics.
In `@src/ngx_oidc_json.c`:
- Around line 37-150: The validation uses hardcoded limits inside
ngx_oidc_json_validate (max_string_length=4096, max_array_size=100,
max_object_keys=256) that can reject valid JWKS/metadata (e.g. x5c); replace
these static consts with configurable values from the module config (add nginx
directives and fields in the module config struct, e.g.,
oidc_json_max_string_length, oidc_json_max_array_size,
oidc_json_max_object_keys) and read those config values in
ngx_oidc_json_validate instead of the constants (fall back to raised safe
defaults, e.g. string 16384, array 1000, object_keys 4096), wiring directive
parsing to populate the config and using those config fields wherever
ngx_oidc_json_validate is called (preserve existing error logs but include the
configured limits in messages).
In `@src/ngx_oidc_json.h`:
- Around line 13-20: Change the opaque JSON handle from a raw void pointer to an
incomplete struct type so the public API using ngx_oidc_json_t * remains a
pointer to an opaque object: replace "typedef void *ngx_oidc_json_t;" with
"typedef struct ngx_oidc_json_s ngx_oidc_json_t;" and keep function signatures
such as ngx_oidc_json_parse(...) returning ngx_oidc_json_t *; update all
internal casts/usages in src/ngx_oidc_json.c (and any callers) that assumed
void** semantics—search for ngx_oidc_json_t usages and NGX_OIDC_JSON_CAST and
adjust them to cast to/from struct ngx_oidc_json_s* (or remove extra
indirection) so code no longer treats the handle as void**; run the provided
grep script to verify there are no non-pointer ngx_oidc_json_t usages remaining.
In `@src/ngx_oidc_jwks.c`:
- Around line 1596-1599: The traversal in ngx_oidc_jwks_clear_fetch_flag
incorrectly branches solely on the hash (using (hash < shm_node->key_hash) ?
node->left : node->right), causing hash-collision errors; update the comparison
so when hash == shm_node->key_hash you perform a byte-wise URI comparison using
ngx_memn2cmp against shm_node->key (and key length) to decide left/right,
otherwise keep the existing hash < shm_node->key_hash branch—modify the
traversal logic in ngx_oidc_jwks_clear_fetch_flag to use ngx_memn2cmp for
equal-hash disambiguation.
- Around line 1119-1124: In jwks_shm_save the rbtree traversal uses "hash <
shm_node->key_hash" even when hash == shm_node->key_hash, which forces
node->right and breaks collision ordering used by jwks_rbtree_insert_value;
change the branch to perform the same URI-based comparison used in
jwks_shm_lookup: when hash == shm_node->key_hash call ngx_memn2cmp against the
stored key (use shm_node->key_len and the incoming uri/len) and choose node =
node->left or node->right based on that string comparison so colliding keys are
ordered consistently with jwks_rbtree_insert_value.
- Around line 1489-1541: Replace the hash-only comparisons and the unconditional
fetch lock set to avoid collisions and TOCTOU races: when walking the rbtree
(both where it currently does `hash < shm_node->key_hash`) use
ngx_memn2cmp(shm_node->jwks_uri.data, jwks_uri->data, shm_node->jwks_uri.len,
jwks_uri->len) to decide left/right/equal so nodes with equal hash but different
URIs are distinguished; after re-locking and locating the candidate node
(jwks_shm_node_t *shm_node) check that the URI matches and then check
shm_node->fetching before setting it — if fetching == 0 set it to 1 and return
NGX_OK, otherwise ngx_shmtx_unlock(&shm->shpool->mutex) and return a suitable
status (e.g., NGX_BUSY) so you do not overwrite another worker's claim; keep
jwks_shm_save and the unlock/lock sequence but add these extra
comparisons/conditional before assigning shm_node->fetching.
In `@src/ngx_oidc_jwt.c`:
- Around line 293-335: The code only reads the first element of an "aud" array;
instead iterate all elements returned by ngx_oidc_json_array_size and use
ngx_oidc_json_array_get to check each element with ngx_oidc_json_is_string,
comparing the string value to the expected client id (the RP's client_id/config)
instead of only taking index 0; when a match is found allocate claims->audience,
copy that matching string (using ngx_pcalloc and ngx_memcpy as done) and stop;
if no element matches, log an error and ngx_oidc_json_free(root) then return
NGX_ERROR. Ensure you reuse the existing functions ngx_oidc_json_object_get,
ngx_oidc_json_is_array, ngx_oidc_json_array_get and handle NULL/memory failures
exactly like the current branches.
In `@src/ngx_oidc_metadata.c`:
- Around line 404-427: The rollback logic in the allocation loop for
ngx_slab_alloc_locked (new_data array in the code handling
src_fields/dest_fields) assumes contiguous allocations via allocated_count and
can leak when some new_data[i] remain NULL; change the failure cleanup to
iterate over the full new_data[] array (all 6 slots) and call
ngx_slab_free_locked(shm->shpool, new_data[j]) for each j where new_data[j] !=
NULL, and remove or update reliance on allocated_count for freeing so every
successful allocation is explicitly freed on error; ensure the error path still
logs using ngx_log_error and leaves new_data entries consistent for the caller.
- Around line 888-917: After re-locking the shared pool and locating the
matching cache_node (after ngx_memn2cmp), re-check cache_node->fetching before
unconditionally setting it: if cache_node->fetching is already non-zero,
ngx_shmtx_unlock(&shm->shpool->mutex) and return a non-fetching result (e.g.,
NGX_DECLINED or other existing sentinel used by this code path) to avoid
duplicate fetches; only set cache_node->fetching = 1 and proceed to log/return
NGX_OK when it was previously zero. Ensure you use the existing symbols
cache_node, fetching, ngx_shmtx_lock/ngx_shmtx_unlock and preserve the current
log/return behavior when acquiring the fetch lock.
In `@src/ngx_oidc_session.c`:
- Around line 331-434: The function ngx_oidc_session_get_temporary_id currently
parses r->headers_in.cookie manually and only inspects the first Cookie header;
replace that manual parsing with the existing helper ngx_oidc_http_cookie_get to
retrieve the cookie value for the fixed name NGX_OIDC_SESSION_CALLBACK, then
perform the same "provider:session_id" split on the returned ngx_str_t value
(use ngx_strlchr to find ':'), allocate a new ngx_str_t and copy only the
session_id portion into r->pool (handle the no-colon case by copying the whole
value), and preserve all current NULL checks and memory allocation error returns
used in ngx_oidc_session_get_temporary_id.
- Around line 436-530: ngx_oidc_session_get_permanent_id currently reimplements
manual cookie parsing and only reads the first Cookie header; replace the manual
loop with the same helper used in get_temporary_id (either call
ngx_oidc_http_cookie_get or use ngx_http_parse_multi_header_lines to collect
cookies) so all Cookie headers are considered, preserve the existing behavior of
resolving provider->cookie_name via ngx_http_complex_value and returning an
allocated ngx_str_t* with the session id, and ensure memory allocation and
null-termination remain correct (refer to ngx_oidc_session_get_permanent_id and
ngx_oidc_http_cookie_get/get_temporary_id to mirror the refactor).
---
Nitpick comments:
In `@Dockerfile`:
- Around line 53-63: The image is currently running as root; update the
Dockerfile (the stage starting with "FROM nginx AS module") to create and switch
to a non-root user and ensure nginx runs on an unprivileged port or with minimal
capabilities: add a non-root user/group, chown the nginx config and any runtime
directories modified by the RUN block (including /etc/nginx and any cache or log
dirs touched during the RUN --mount block), update nginx.conf (the file
referenced by the sed line that inserts load_module
"/usr/lib/nginx/modules/ngx_http_oidc_module.so") to listen on an unprivileged
port (e.g., 8080) or adjust the container runtime to drop privileges, and add a
USER <nonroot> directive in the final image stage so the container does not run
as root. Ensure file permissions are set so the non-root user can read the
module and write to logs/temp.
In `@src/ngx_oidc_handler_status.h`:
- Around line 13-19: Update the `@return` doc in the ngx_oidc_handler_status.h
comment to match the actual return values used by the implementation (see
ngx_oidc_handler_status.c); replace the generic "NGX_OK or NGX_ERROR" with the
specific HTTP status codes returned by the handler (e.g., NGX_HTTP_OK,
NGX_HTTP_NOT_ALLOWED, NGX_HTTP_INTERNAL_SERVER_ERROR) and a short note that the
function returns an ngx_int_t HTTP status code per ngx_http handler conventions.
In `@src/ngx_oidc_jwt.c`:
- Around line 1338-1345: The code uses ngx_strstr(algorithm, "...") which can
match unintended strings; update the logic in ngx_oidc_jwt.c where hash_half_len
is set (variable algorithm) to use explicit comparisons against known JWT alg
values (e.g. "HS256","RS256","ES256","PS256" -> hash_half_len=16;
"HS384","RS384","ES384","PS384" -> 24; and
"HS512","RS512","ES512","PS512","EdDSA" -> 32) instead of substring searches;
implement these checks with exact string comparisons (ngx_strcmp/ngx_strncmp or
equivalent) to ensure only the intended algorithms set each truncation length.
In `@src/ngx_oidc_random.h`:
- Around line 13-81: Update the function docs to specify buffer
ownership/lifetime: for ngx_oidc_random_state, ngx_oidc_random_nonce,
ngx_oidc_random_code_verifier, ngx_oidc_random_code_challenge, and
ngx_oidc_random_session_id state that on success (NGX_OK) the returned
ngx_str_t->data is allocated from the request pool (r->pool), is NUL-terminated
if applicable, must not be freed by the caller, and remains valid until the
request pool is destroyed; also state that on failure the output is
undefined/unchanged. Add this ownership contract to each function comment in
ngx_oidc_random.h.
In `@src/ngx_oidc_session_store_memory.c`:
- Around line 522-524: The get path is doing unnecessary queue reordering:
remove the calls to ngx_queue_remove(&ocn->queue) and
mem_queue_insert_expire(&octx->expire_queue, ocn) in the get operation since
ocn->expires is not modified and reinsertion will place the node in the same
spot, holding the lock longer; if you intended LRU semantics instead, update
ocn->expires before reinserting and then call ngx_queue_remove +
mem_queue_insert_expire, otherwise delete those two lines and any comments
referencing reordering to avoid needless work.
In `@src/ngx_oidc_session.c`:
- Around line 171-292: ngx_oidc_session_rotate can leave partially-moved data in
the new session when a later ngx_oidc_session_set fails; modify
ngx_oidc_session_rotate so that on any failure after a successful
ngx_oidc_session_set it calls ngx_oidc_session_delete for the same keys on
new_session_id to roll back partial moves (delete NGX_OIDC_SESSION_KEY_ID_TOKEN,
NGX_OIDC_SESSION_KEY_ACCESS_TOKEN, NGX_OIDC_SESSION_KEY_REFRESH_TOKEN,
NGX_OIDC_SESSION_KEY_USERINFO as applicable), keep error logging via
r->connection->log, and return NGX_ERROR; implement the cleanup in each failure
branch inside ngx_oidc_session_rotate (use the same ngx_oidc_session_delete
helper used for old_session_id).
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores