chore(main): release 0.8.0#1385
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1385 +/- ##
=====================================
Coverage 5.04% 5.04%
=====================================
Files 89 89
Lines 13194 13194
=====================================
Hits 666 666
+ Misses 12462 12461 -1
- Partials 66 67 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
193b008 to
d016e9d
Compare
b5b503e to
d44d7ed
Compare
There was a problem hiding this comment.
💬 Comment - Found 2 suggestions for improvement
This is an automated release PR generated by release-please. The changelog update looks mostly good, but I found a couple of minor issues:
Issues Found
1. Duplicate changelog entries (CHANGELOG.md)
Several bug fixes are listed twice - once for the merge commit and once for the actual commit. While both are technically valid git commits, having duplicates makes the changelog harder to read:
- Lines 30-31: "Skip latest-release job on pull requests" (commits 2228f55 and 9855cd6)
- Lines 42-43: "fix critical bugs in IPv6 support implementation" (commits 2261567 and 27e3d6b)
- Lines 46-47: "enable errcheck linter and fix all unchecked errors" (commits 72c7e9a and 2835f12)
- Lines 61-62: "update GitHub Actions config to resolve build failures after Go 1.25 upgrade" (commits 01b2fc1 and e5ce8be)
- Lines 58-59: "Upgrade golang.org/x/crypto to v0.46.0 in examples/server_plugin" (commits 7f3934a and dfb1ee3)
- Lines 63-64: "use format specifiers in fmt.Errorf calls" (commits cfa028d and c797179)
Recommendation: Configure release-please to exclude merge commits or deduplicate entries with the same description.
2. Unreleased section link not updated (CHANGELOG.md:128)
The [Unreleased] reference link still points to v0.6.0...HEAD but should be updated to v1.0.0...HEAD now that 1.0.0 is being released:
-[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEAD
+[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v1.0.0...HEADNon-Issues
- The breaking change warning for the crypto cipher scheme change is properly documented ✓
- All commit references are valid and point to real commits ✓
- The version comparison link (v0.6.0...v1.0.0) is correct ✓
- The release date (2026-01-17) matches today's date ✓
- Mixed formatting (inline links for 1.0.0 vs reference-style for older versions) is release-please's standard behavior ✓
Recommendation
These are minor quality-of-life improvements. Since this is an auto-generated release PR, consider updating the release-please configuration to handle these edge cases in future releases.
ed403da to
784de56
Compare
There was a problem hiding this comment.
🚨 Changes Requested - Found 1 issue that must be addressed
This is an auto-generated release PR from Release Please. The changelog content looks comprehensive and accurately documents the v1.0.0 release, including proper documentation of the breaking change in the crypto cipher scheme defaults.
Issues Found
CHANGELOG.md:128-133 - Missing version link definitions
The changelog is missing the link definition for the new v1.0.0 release, and the [Unreleased] link needs to be updated to point to v1.0.0 instead of v0.6.0.
Current state:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEAD
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0Should be:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v1.0.0...HEAD
[1.0.0]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...v1.0.0
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0What's Good
- ✅ All commit hashes are valid and verified
- ✅ Breaking change properly documented with clear migration instructions
- ✅ Release date (2026-01-18) matches today's date
- ✅ Semantic versioning correctly applied (major version bump for breaking change)
- ✅ Follows Keep a Changelog format
- ✅ Comprehensive documentation of features, bug fixes, and security improvements
- ✅ PR follows conventional commit format
This is a standard release process issue - once the link definitions are updated, the release will be ready to merge.
784de56 to
f52309a
Compare
There was a problem hiding this comment.
✅ Approved - Code looks good
This is an automated release PR from Release Please that adds the v1.0.0 release section to the CHANGELOG.md.
Review Summary
Verified:
- All commit references are valid and exist in the repository
- Breaking change for crypto cipher scheme is properly documented with clear migration instructions
- Date format is correct (2026-01-19 matches today's date)
- Formatting follows Keep a Changelog conventions
- Links to commits, PRs, and issues are properly formatted
- Security fixes are appropriately documented
- Version comparison link is correctly formed (v0.6.0...v1.0.0)
Notable items in this release:
- Breaking Change: Default cipher scheme changed from GMSM (value 0) to CURVE (value 0). Existing configs need to set value to 1 for GMSM.
- 7 new features including CLI JSON output, startup banners, and plugin build support
- 46 bug fixes covering security, IPv6 support, CI/CD, crypto error handling, and more
- Multiple security improvements: secure cookies, crypto upgrades, master key permissions, bounds checking
The changelog is comprehensive and ready for the v1.0.0 release.
There was a problem hiding this comment.
🚨 Changes Requested - Found 1 critical issue that must be addressed
Critical Issue
VERSION file not updated (CHANGELOG.md:8)
The CHANGELOG documents this as a v1.0.0 release, but the version file at nhp/version/VERSION still contains 0.6.0. This creates a version mismatch that could cause:
- Build inconsistency: The version placeholders in
nhp/version/version.goare populated at build time from the VERSION file or git tags, so binaries may report incorrect versions - Release automation issues: If the release process relies on the VERSION file, it may fail or create the wrong release
- User confusion: Documentation and runtime version reporting will be inconsistent
Required fix: Update nhp/version/VERSION from 0.6.0 to 1.0.0 to match the CHANGELOG.
What Looks Good
✅ Breaking change properly documented: The crypto cipher scheme change is clearly explained with migration instructions
✅ Comprehensive changelog: All features, fixes, and changes are well-documented with commit references
✅ Follows Keep a Changelog format: Proper semantic versioning structure
✅ Security fixes included: Multiple security improvements (cookies, crypto upgrades, bounds checking)
Please update the VERSION file and I'll re-review.
c8308c7 to
5e238a1
Compare
There was a problem hiding this comment.
🚨 Changes Requested - Found 1 critical issue that must be addressed
Critical Issue: Missing Version Reference Links
CHANGELOG.md:139-144 - The version comparison links at the bottom of the file are incomplete:
Problems:
- Missing the
[1.0.0]version link definition - The
[Unreleased]link still points tov0.6.0...HEADinstead ofv1.0.0...HEAD
Expected:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v1.0.0...HEAD
[1.0.0]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...v1.0.0
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0
...Current (incorrect):
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEAD
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0
...Without this fix, the [1.0.0] link on line 7 will be broken.
Minor Issue (Suggestion)
CHANGELOG.md:7 vs 79 - Date format inconsistency:
- Line 7 uses:
(2026-01-20)(parentheses) - Line 79 uses:
- 2025-06-11(dash prefix)
While not critical, consider using the same format throughout for consistency. Release Please typically uses parentheses format.
5e238a1 to
e180c8a
Compare
There was a problem hiding this comment.
✅ Approved - Changelog update looks good
This auto-generated Release Please PR properly documents the v1.0.0 release with:
- ✅ Properly formatted changelog following Keep a Changelog standards
- ✅ Breaking change clearly documented with migration path
- ✅ Comprehensive list of 10 features and 50+ bug fixes with commit references
- ✅ Semantic versioning correctly applied (v0.6.0 → v1.0.0 due to breaking change)
- ✅ All entries include proper commit hashes and PR references
The changelog comprehensively captures security improvements (bounds checking, crypto upgrades, secure cookies), critical bug fixes (IPv6 support, error handling improvements), and new functionality (CLI enhancements, deployment workflows).
No issues found. Ready to merge.
There was a problem hiding this comment.
💬 Comment - Found 2 minor issues that should be addressed
Issues Found
1. Missing Version Link Reference (CHANGELOG.md:145)
The changelog is missing the link reference for version 1.0.0 at the bottom of the file.
Expected:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v1.0.0...HEAD
[1.0.0]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...v1.0.0
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0Currently:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEAD
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0This breaks the clickable version links following the Keep a Changelog format.
2. Duplicate Changelog Entries (CHANGELOG.md:38-39, 54-55, 58-59, 74-75, 78-81)
The changelog contains duplicate entries for the same logical changes:
- Lines 38-39: "Skip latest-release job on pull requests" appears twice
- Lines 54-55: "fix critical bugs in IPv6 support implementation" appears twice
- Lines 58-59: "enable errcheck linter and fix all unchecked errors" appears twice
- Lines 74-75: "Upgrade golang.org/x/crypto to v0.46.0" appears twice
- Lines 78-79: "update GitHub Actions config to resolve build failures after Go 1.25 upgrade" appears twice
- Lines 80-81: "use format specifiers in fmt.Errorf calls" appears twice
These duplicates are likely from including both merge commits and their associated PR commits. While technically correct from git history, it creates noise in the changelog. Consider consolidating these into single entries.
Positive Notes
- Breaking change is properly documented and clearly explained
- Security fixes are appropriately labeled
- Follows Keep a Changelog format conventions
- Good categorization of features vs bug fixes
- Comprehensive list of changes for a major version bump
Recommendation
These are minor documentation issues that don't affect functionality. The release-please automation may handle the missing link reference automatically, but it would be cleaner to verify this before merging.
cd8153e to
24d9c82
Compare
There was a problem hiding this comment.
🚨 Changes Requested - Found 2 issues that must be addressed
This is an automated release PR that updates CHANGELOG.md for the 1.0.0 release. While the changelog entries themselves are comprehensive and well-formatted, there are critical issues with the version comparison links that must be fixed.
🔴 BREAKING CHANGES - Missing Version Links (CRITICAL)
Issue: The version comparison links at the bottom of CHANGELOG.md are incomplete and incorrect.
Location: CHANGELOG.md:145-150
Current state:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEAD
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0
...Required changes:
- The
[Unreleased]link should compare fromv1.0.0...HEAD, notv0.6.0...HEAD - A new link for
[1.0.0]must be added
Should be:
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v1.0.0...HEAD
[1.0.0]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...v1.0.0
[0.6.0]: https://github.com/OpenNHP/opennhp/compare/v0.5.0...v0.6.0
...🟡 CODE QUALITY - Duplicate Changelog Entries (MINOR)
Issue: Several entries appear twice with identical descriptions but different commit hashes. While this might be intentional if these are separate commits addressing the same issue, it creates visual clutter in the changelog.
Affected entries:
- Lines 54-55: Two "ipv6: fix critical bugs in IPv6 support implementation" entries
- Lines 57-58: Two "lint: enable errcheck linter and fix all unchecked errors" entries
- Lines 37-38: Two "ci: Skip latest-release job on pull requests" entries
- Lines 73-74: Two "security: Upgrade golang.org/x/crypto to v0.46.0" entries
- Lines 77-78: Two "update GitHub Actions config to resolve build failures" entries
- Lines 79-80: Two "utils: use format specifiers in fmt.Errorf calls" entries
Recommendation: Consider consolidating duplicate entries into a single line, or if these truly are separate commits that should both be listed, add distinguishing context to make it clear why both are included.
✅ What Looks Good
- Breaking changes are clearly documented with the
⚠ BREAKING CHANGESsection - The cipher scheme change is well explained
- All commit references and PR links are properly formatted
- Follows Keep a Changelog format conventions
- Comprehensive coverage of all changes since v0.6.0
24d9c82 to
280ea53
Compare
There was a problem hiding this comment.
✅ Approved - Code looks good
This is an automated Release Please PR for version 1.0.0 that updates the CHANGELOG.md with a comprehensive list of changes from v0.6.0 to v1.0.0.
Review Summary
Changes:
- Adds changelog entry for v1.0.0 release (76 additions, 1 deletion)
- Documents 10 new features and 46 bug fixes
- Properly highlights breaking change for crypto cipher scheme default
Security:
- ✅ Breaking change is clearly documented with migration instructions
- ✅ Multiple security fixes are properly documented (bounds checking, secure cookies, crypto upgrades, key file permissions)
- ✅ No executable code changes - documentation only
- ✅ All commit references are properly linked
Quality:
- ✅ Follows Keep a Changelog format
- ✅ Properly structured with conventional commit categories
- ✅ Date is correct (2026-01-27)
- ✅ Version links are properly formatted
This is a standard release changelog update generated by Release Please. The breaking change warning is appropriately prominent and includes clear migration guidance.
There was a problem hiding this comment.
✅ Approved - Code looks good
This is an automated Release Please PR for version 1.0.0 that updates the CHANGELOG.md with a comprehensive list of changes from v0.6.0 to v1.0.0.
Review Summary
Changes:
- Adds changelog entry for v1.0.0 release (76 additions, 1 deletion)
- Documents 10 new features and 46 bug fixes
- Properly highlights breaking change for crypto cipher scheme default
Security:
- ✅ Breaking change is clearly documented with migration instructions
- ✅ Multiple security fixes are properly documented (bounds checking, secure cookies, crypto upgrades, key file permissions)
- ✅ No executable code changes - documentation only
- ✅ All commit references are properly linked
Quality:
- ✅ Follows Keep a Changelog format
- ✅ Properly structured with conventional commit categories
- ✅ Date is correct (2026-01-27)
- ✅ Version links are properly formatted
This is a standard release changelog update generated by Release Please. The breaking change warning is appropriately prominent and includes clear migration guidance.
a98b9b0 to
25cd07e
Compare
There was a problem hiding this comment.
🚨 Changes Requested - Found 13 issue(s) that must be addressed
This is a Release-Please PR for v1.0.0. The CHANGELOG diff itself is fine, but the code it tags contains several issues — including critical security vulnerabilities — that should be resolved before cutting a stable 1.0.0 release.
Critical Issues
1. Hardcoded session cookie signing secret
endpoints/server/httpserver.go:77
store := cookie.NewStore([]byte("nhpstore"))The static string "nhpstore" is the signing key for all session cookies. Anyone who reads this source can forge or decrypt sessions, defeating the CSRF protection on the OIDC flow (state, oauth_token, profile are all session-stored). Fix: Generate a cryptographically random 32-byte key at startup, or load it from config/Secrets Manager.
2. Wildcard CORS with Allow-Credentials: true
endpoints/server/httpserver.go:301-305
c.Writer.Header().Set("Access-Control-Allow-Origin", "*")
c.Writer.Header().Set("Access-Control-Allow-Credentials", "true")This combination is spec-invalid and signals a misconfiguration. If a proxy echoes back the request Origin header instead, this allows any cross-origin site to make credentialed requests with the session cookie. The OIDC plugin already has a correct allowlist-based CORS middleware (examples/server_plugin/oidc/main.go:527-548) — the server should mirror that approach.
High Severity Issues
3. log.Fatalf (process kill) and nil dereference in SM2 crypto
nhp/core/crypto.go:325-328 and :299
SM2Decrypt calls log.Fatalf on a bad private key, which calls os.Exit(1) and terminates the entire server — a DoS vector reachable via malformed config or a config-reload race. Additionally, in SM2Encrypt the error from Base64DecodeSM2ECDSAPublicKey is never checked; if it fails, the nil sm2PublicKey is passed directly to sm2.EncryptASN1, causing a panic. Fix: Return errors instead of fataling; add nil check after the base64 decode.
4. Data race on global oidcAuth pointer
examples/server_plugin/oidc/main.go:108, 314-334
The package-level var oidcAuth *Authenticator is written in authOidc and read in authRegular/authAndShowLogin under concurrent gin request handlers with no mutex or atomic. This is a data race that can cause nil-pointer panics or read-tearing. Fix: Protect with a sync.RWMutex or use atomic.Pointer[Authenticator].
5. Swallowed hex.DecodeString error in SM2Decrypt
nhp/core/crypto.go:317-322
ciphertext, err := hex.DecodeString(message)
privKeyBytes, err := base64.StdEncoding.DecodeString(privateKeyBase64) // overwrites err
if err != nil { ... }The := on the second line shadows the first error. A hex-decode failure silently continues with a nil ciphertext, causing a downstream panic or silent corruption. Fix: Use separate err variables and check each immediately.
6. Breaking: cipher scheme default changed without migration tooling
nhp/common/packet.go:16-19
CIPHER_SCHEME_GMSM was previously 0 (iota default); it is now 1. Any existing installation with DefaultCipherScheme = 0 (or an omitted field) silently switches from GMSM to CURVE on upgrade. Packets are cipher-incompatible; all agent-server handshakes break silently. The BREAKING CHANGES section in the changelog documents this but provides no migration tooling or startup warning. Fix: Add a startup-time log warning when DefaultCipherScheme == 0, clearly stating which cipher is now active. A short migration guide in the RUNBOOK would also help operators.
Medium Severity Issues
7. sessionSave errors silently ignored after token storage
examples/server_plugin/oidc/main.go:359-360, 391-392
If sessionSave fails after writing oauth_token/profile, the stale uncleared state remains in the cookie, enabling CSRF-state replay. Return an error response if sessionSave fails.
8. Unencoded state appended to redirect URL
examples/server_plugin/oidc/main.go:306
Use url.Values.Encode() when building redirect URLs to prevent URL injection from edge-case state values.
9. Debug fmt.Printf of ciphertext left in production code
nhp/core/crypto.go:313
fmt.Printf("Ciphertext: %x\n", ciphertext)Outputs sensitive ciphertext to stdout on every SM2Encrypt call. Remove this line.
10. HTML template overwritten on every file in loop
endpoints/server/httpserver.go:185-191
g.SetHTMLTemplate(t) is called inside the WalkDir loop, replacing the entire template set with each file. Only the last file is retained; rendering any earlier template will panic with "template not defined". Fix: Parse all files into a single rootTmpl, then call g.SetHTMLTemplate(rootTmpl) once after the walk.
11. Wildcard CORS on relay endpoint
endpoints/relay/relay.go:609-623
Access-Control-Allow-Origin: * allows any browser origin to initiate relay requests, which could be used to relay NHP knock packets using a victim's source IP. Recommend adding a configurable AllowedOrigins list and documenting the threat model in a comment.
Low Severity Issues
12. PKCS#7 unpadding skipped for block-aligned plaintext
nhp/core/crypto.go:280-289
PKCS#7 always adds a full padding block even when data is block-aligned. The conditional that skips unpadding for len(plaintext)%blockSize == 0 will return data with a trailing padding block, silently corrupting decrypted output. The AESDecrypt function (line 386) correctly calls unpad unconditionally — make CBCDecryption consistent.
13. CHANGELOG format inconsistency
CHANGELOG.md:8, 246
The 1.0.0 section uses a plain ## 1.0.0 header (not ## [1.0.0]) and has no footer comparison link. The [Unreleased] link still points to v0.6.0...HEAD. Add [1.0.0]: .../compare/v0.6.0...v1.0.0 to the footer and update [Unreleased] to compare from v1.0.0.
Issues 1–6 (hardcoded secret, wildcard CORS+credentials, SM2 DoS/panic, global race, swallowed error, silent cipher migration) should be fixed before tagging v1.0.0 as stable.
25cd07e to
6793f6e
Compare
There was a problem hiding this comment.
🚨 Changes Requested - Found 15 issue(s) that must be addressed
This is a Release Please PR for v1.0.0. The diff itself is only CHANGELOG.md, but the review must cover the code being shipped as a stable release.
Recommendation: Block this 1.0.0 release until at minimum issues 1.1 and 1.3 below are resolved. Issues 1.2, 1.5, 2.3, and 2.4 should also be fixed before tagging a stable release.
Severity 1 — Security Vulnerabilities
1.1 Hard-coded session secret (endpoints/server/httpserver.go:77) — Critical
store := cookie.NewStore([]byte("nhpstore"))This static, publicly-visible secret allows any attacker to forge valid session cookies for any nhp-server deployment, enabling session hijacking — including hijacking the OIDC state value that guards CSRF protection.
Fix: Derive the session secret from the server's private key at startup (e.g., HKDF-SHA256(privateKey, "nhp-session-secret")), or load it from the config file. Never hard-code it.
1.2 Incompatible CORS + Credentials (endpoints/server/httpserver.go:301,305) — High
c.Writer.Header().Set("Access-Control-Allow-Origin", "*")
c.Writer.Header().Set("Access-Control-Allow-Credentials", "true")The RFC (and all browsers) prohibit Access-Control-Allow-Credentials: true with Access-Control-Allow-Origin: *. Browsers silently ignore this combination, so Set-Cookie headers are never forwarded to cross-origin callers. The OIDC plugin's cookie-based auth is effectively broken for browser-based agents making cross-origin requests.
Fix: Reflect the Origin header (as the OIDC plugin's own corsMiddleware does) against an allowlist, instead of echoing *.
1.3 log.Fatalf crashes server process on malformed input (nhp/core/crypto.go:327) — High
testkey, err := sm2.NewPrivateKey(privKeyBytes)
if err != nil {
log.Fatalf("fail to new private key %v", err)
}log.Fatalf calls os.Exit(1), terminating the entire server process. If SM2Decrypt is ever called with attacker-controlled input (e.g., via an API or config), this is a trivially-triggered Denial of Service. Additionally, the previous line shadows the err from hex.DecodeString, so a corrupted ciphertext value is silently used if hex decode fails.
Fix: Return an error instead of calling log.Fatalf. Fix the shadowed-error pattern on line 319–320.
1.4 Error from Base64DecodeSM2ECDSAPublicKey silently dropped (nhp/core/crypto.go:299) — High
sm2PublicKey, err := gmsm.Base64DecodeSM2ECDSAPublicKey(pubKeyBase64)
// err is immediately overwritten by the next :=
ciphertext, err := sm2.EncryptASN1(rng, sm2PublicKey, secretMessage)If the public key decode fails, sm2PublicKey is nil/zero and sm2.EncryptASN1 will panic or produce garbage. The caller gets unexpected behavior without any error signal.
Fix: Check the error before proceeding.
1.5 Data race on oidcAuth global (examples/server_plugin/oidc/main.go) — High
oidcAuth is a package-level variable shared across goroutines (gin routes are served concurrently) but read and written without any synchronization:
- Written at lines 316/319 (
authOidc) - Read at line 304 (
authAndShowLogin) and lines 333, 369, 376, 404 (authRegular)
A concurrent ?action=oauth and ?action=valid request can race: one goroutine reads oidcAuth after another sets it to nil, causing a nil-pointer panic. resourceMapMutex protects the resource map but nothing protects oidcAuth.
Fix: Protect oidcAuth with a sync.RWMutex or replace it with an atomic.Pointer[Authenticator].
Severity 2 — Bugs and Logic Errors
2.1 CBCEncryption output buffer allocated with wrong size (nhp/core/crypto.go:226) — Medium
ciphertext = make([]byte, 0, len(plaintext)) // capacity = len(plaintext), not len(paddedPlainText)
// ...
mode.CryptBlocks(ciphertext, paddedPlainText) // writes to a zero-length slice → no-opWhen inPlace is false, the buffer is allocated with capacity len(plaintext) but CryptBlocks writes to ciphertext[0:] (zero length), producing an empty ciphertext silently. The correct allocation is make([]byte, len(paddedPlainText)).
2.2 sessionSave errors silently ignored (examples/server_plugin/oidc/main.go:360,392) — Medium
Both calls to sessionSave(ctx) at lines 360 and 392 discard the error return value. If the session cannot be persisted, CSRF state-clearing silently fails (protection is not cleared) and token storage silently fails (user gets a confusing empty response).
2.3 Template overwrite in loop — only last template survives (endpoints/server/httpserver.go:185–191) — Medium
// Inside WalkDir loop:
t := rootTmpl.New(path)
_, err = t.Parse(string(content))
g.SetHTMLTemplate(t) // replaces the entire template set on every iterationg.SetHTMLTemplate(t) is called once per file inside the loop. Each call replaces the engine's entire template set with a single-file template. Only the last file walked is actually registered; all others are silently lost.
Fix: Build the full rootTmpl outside the loop and call SetHTMLTemplate once after all files are parsed.
2.4 Breaking change (cipher scheme swap) not reflected in Chinese docs — Medium
docs/zh-cn/remo_config.zh-cn.md (lines 99, 215) still documents the old mapping (0 = GMSM, 1 = CURVE). After the breaking change in #1330, 0 = CURVE and 1 = GMSM. Operators reading this document will silently get the wrong cipher scheme — a security downgrade for organizations required to use GM/T standards.
2.5 TOCTOU race in stale relay connection cleanup (endpoints/server/msghandler.go:748–762) — Medium
The lock is released between the "find" and "delete" operations. Two concurrent goroutines can both find the same stale connection, both delete it, and both decrement relayConnCount, causing a double-decrement (potential underflow) that permanently denies future connections from that relay.
Fix: Perform the find, delete, and counter update under a single lock acquisition.
Severity 4 — Performance
4.1 Unbounded pendingRequests map in relay (endpoints/relay/relay.go) — Low
Under a slow or unresponsive NHP server, every in-flight request adds an entry with no size cap. Under sustained load, this can cause unbounded memory growth in the relay process.
Severity 5 — Code Quality
5.1 Ciphertext leaked to stdout in production (nhp/core/crypto.go:307–332) — Low
fmt.Printf("Ciphertext: %x\n", ciphertext) // leaks full ciphertext to stdoutDebug fmt.Printf/fmt.Fprintf calls remain in the production crypto path. The full ciphertext is printed to stdout on every SM2Encrypt call. Use the project's structured logger instead.
5.2 Stdlib "log" imported alongside project logger (nhp/core/crypto.go:13) — Low
The stdlib log.Fatalf on line 327 bypasses the project's log infrastructure and cannot be suppressed in tests. Import log "github.com/OpenNHP/opennhp/nhp/log" instead.
5.3 Nil dereference on oidcAuth in authAndShowLogin (examples/server_plugin/oidc/main.go:304) — Low
authAndShowLogin calls oidcAuth.VerifyIDToken(...) at line 304 without a nil guard, but oidcAuth can be nil before the first OAuth flow is initiated. authRegular has a nil guard; authAndShowLogin does not.
5.4 Import grouping non-compliant with CLAUDE.md (nhp/core/crypto.go) — Low
Per project conventions, imports must be in three groups (stdlib / external / internal). The current block mixes stdlib ("log", "os") with the external group.
Summary
| # | File | Severity | Issue |
|---|---|---|---|
| 1.1 | endpoints/server/httpserver.go:77 |
Critical | Hard-coded session secret — cookie forgery |
| 1.2 | endpoints/server/httpserver.go:301,305 |
High | Wildcard CORS + credentials breaks cookie delivery |
| 1.3 | nhp/core/crypto.go:327 |
High | log.Fatalf in SM2Decrypt — DoS via process crash |
| 1.4 | nhp/core/crypto.go:299 |
High | Error from key decode silently dropped |
| 1.5 | examples/server_plugin/oidc/main.go |
High | Data race on oidcAuth global |
| 2.1 | nhp/core/crypto.go:226 |
Medium | CBCEncryption zero-length output — silent no-op |
| 2.2 | examples/server_plugin/oidc/main.go:360,392 |
Medium | sessionSave errors ignored |
| 2.3 | endpoints/server/httpserver.go:191 |
Medium | Template overwritten per-file in loop |
| 2.4 | docs/zh-cn/remo_config.zh-cn.md:99,215 |
Medium | Cipher scheme swap not reflected in docs |
| 2.5 | endpoints/server/msghandler.go:748–762 |
Medium | TOCTOU race in relay cleanup |
| 4.1 | endpoints/relay/relay.go |
Low | Unbounded pending-requests map |
| 5.1 | nhp/core/crypto.go:307–332 |
Low | Ciphertext leaked to stdout |
| 5.2 | nhp/core/crypto.go:13 |
Low | Stdlib "log" bypasses project logger |
| 5.3 | examples/server_plugin/oidc/main.go:304 |
Low | Nil dereference in authAndShowLogin |
| 5.4 | nhp/core/crypto.go |
Low | Import grouping non-compliant |
6793f6e to
29caef2
Compare
There was a problem hiding this comment.
💬 Comment - Found 3 minor suggestion(s) for improvement
This is a release-please automated PR; only CHANGELOG.md is modified (+175/-1). No source, config, or workflow changes — so security/bugs/breaking-change criteria don't apply to the diff itself. The 0.6.0 → 1.0.0 bump is semver-justified by the documented BREAKING CHANGE (cipher scheme numeric values flipped: DefaultCipherScheme=0 now means CURVE instead of GMSM, per #1330).
A few minor doc nits, none blocking:
-
[Unreleased]placement (CHANGELOG.md:182) — release-please inserts the new## 1.0.0 (2026-05-06)section above the existing## [Unreleased]heading. Per Keep a Changelog (which the file's preamble cites),[Unreleased]should remain at the top so subsequent changes accumulate there. After this PR merges, consider moving## [Unreleased]back above## 1.0.0. -
Format/link-reference inconsistency (CHANGELOG.md:8 and bottom of file) — older releases use
## [0.6.0] - 2025-06-11with a matching[0.6.0]: https://github.com/.../compare/...link reference at the bottom. The new entry uses## 1.0.0 (2026-05-06)(release-please style) and adds no[1.0.0]: ...reference. The headings render fine either way, but the file is now stylistically split. If you want to keep Keep-a-Changelog compatibility, add[1.0.0]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...v1.0.0to the link-reference block. -
Trailing newline removed at EOF (CHANGELOG.md:250) — the diff drops the final newline on the
[0.3.6]: ...line. POSIX text files should end with\n; some editors/tools complain. Worth restoring. -
(Informational, not actionable) — Several adjacent entries are near-duplicates (e.g. "ipv6: fix critical bugs in IPv6 support implementation" appears twice, "Add the OIDC authentication plugin." twice, "security: Upgrade golang.org/x/crypto..." twice). These reflect duplicate commits in the underlying history; release-please is just echoing them faithfully. Not worth fixing in this PR, but a future cleanup could squash duplicates before tagging.
The cipher-scheme breaking change in particular is significant for downstream operators — users with DefaultCipherScheme=0 in existing configs will silently switch crypto suites on upgrade. Worth verifying the upgrade docs/release notes call this out prominently in addition to the CHANGELOG line.
7f64ed1 to
7ca7bea
Compare
7ca7bea to
43c6b81
Compare
43c6b81 to
fc65288
Compare
There was a problem hiding this comment.
💬 Comment - Found 4 suggestion(s) for improvement
This is an automated Release Please PR that bumps CHANGELOG.md to v0.8.0. No code changes, so no security/logic concerns. However, the changelog content has several inconsistencies worth fixing before merging.
Issues
1. Missing v0.7.0 entry — changelog gap (CHANGELOG.md)
The new section is ## [0.8.0](.../compare/v0.7.0...v0.8.0), implying v0.7.0 exists (and indeed it does — commit 32957c2 chore: release 0.7.0 created it as a Release-As bump). But the CHANGELOG jumps directly from v0.8.0 → v0.6.0; v0.7.0 is never documented. Users reading the changelog can't see what shipped in 0.7.0. Either add a ## [0.7.0] stub or amend release-please configuration so the missing release is captured.
2. Duplicate bullet in 0.8.0 section (CHANGELOG.md:13-14)
* **js-agent:** improve demo page DX for SDK integrators ([0e3c561](.../commit/0e3c5617...))
* **js-agent:** improve demo page DX for SDK integrators ([2f718af](.../commit/2f718af2...))
0e3c561 is the merge commit for PR #1527; 2f718af is the feature commit inside that same PR. Same change, listed twice. Consider configuring release-please to drop merge commits (or squash-merging features) so users see one entry per change.
3. "Unreleased" section position (CHANGELOG.md:16)
Per Keep a Changelog (which the file header references), ## [Unreleased] should sit at the top, above the most recent release. The new entry was inserted above ## [Unreleased], leaving Unreleased buried between v0.8.0 and v0.6.0.
4. Stale link reference at file footer (CHANGELOG.md:78-83)
The reference-link table at the bottom was not updated:
[Unreleased]: .../compare/v0.6.0...HEAD— should now compare fromv0.8.0...HEAD- No
[0.8.0]or[0.7.0]reference entries added (the body uses inline links so this is cosmetic, but the existing convention is reference-style) - Trailing newline at EOF was dropped (line 83 of the new file no longer ends with
\n) — minor POSIX hygiene issue.
Summary
None of these block the release, but they make the changelog less useful as a historical record. Worth a follow-up commit to: (a) add a v0.7.0 stub, (b) deduplicate the bullets, (c) move ## [Unreleased] back to the top, and (d) refresh the reference-link table at the footer.
fc65288 to
af04f0c
Compare
There was a problem hiding this comment.
✅ Approved - Code looks good
This is a routine release-please-generated PR. Scope is limited to CHANGELOG.md (14 additions, 1 deletion); no source, config, or workflow changes. No security, correctness, breaking-change, or performance concerns.
Minor observations (non-blocking)
- CHANGELOG.md:5-17 — The two
js-agentfeature entries have identical messages (improve demo page DX for SDK integrators) but reference different commits (0e3c561and2f718af). This is mechanical output from Conventional Commits + release-please, so it's expected, but worth a glance to confirm both commits are intentionally listed (e.g., not a force-push artifact). If you'd like deduped entries in future releases, distinguish the commit subjects. - CHANGELOG.md:88 — The diff drops the trailing newline on the final reference link line (
[0.3.6]: ...). POSIX text files conventionally end with\n; some tooling and thegofmt-equivalent for Markdown linters flag this. Minor cosmetic, won't block CI here since CHANGELOG isn't formatter-gated. - Release date — Tagged
2026-05-07, which matches today. Nothing to fix; just confirming the date isn't stale from a stuck branch.
Nothing else to flag — safe to merge once the release-please tag/release flow runs.
af04f0c to
5f50133
Compare
There was a problem hiding this comment.
🚨 Changes Requested — Found 1 release-correctness bug and 4 CHANGELOG issues.
This is a release-please PR for 0.8.0, but it touches only CHANGELOG.md. Several artifacts that should move with the version are not updated, and the changelog itself violates the Keep-a-Changelog format the file claims to follow (line 5).
1. (Bug, category 3) nhp/version/VERSION is not bumped to 0.8.0
nhp/version/VERSIONstill contains0.7.1.Makefile:19reads it:VERSION = $(shell cat nhp/version/VERSION).$(TIMESTAMP).Makefile:27bakes it into every binary via LD flags:-X '${GOMODULE}/version.Version=${VERSION}'.- It also drives the release tarball name (
Makefile:25).
If this PR merges as-is and is tagged v0.8.0, every released binary will self-report Version=0.7.1.<timestamp> and the artifact will be named opennhp-0.7.1.*.tar.gz. The release-please config (release-type: go in .github/workflows/release-please.yml) does not know about this file — you likely need a release-please-config.json with an extra-files entry for nhp/version/VERSION, or switch the release config so it tracks that file.
2. (Doc bug, category 3) ## [Unreleased] is now in the wrong position
The new ## [0.8.0] block was inserted above the existing ## [Unreleased] section (CHANGELOG.md:8–16), leaving an empty Unreleased section sandwiched between 0.8.0 and 0.6.0. Per Keep-a-Changelog (which the file references), Unreleased must always be the first section.
3. (Doc bug, category 3) CHANGELOG history skips 0.7.x entirely
The file jumps from ## [0.6.0] - 2025-06-11 (line 18) directly to the new ## [0.8.0] (line 8). The compare link in the new entry — compare/v0.7.1...v0.8.0 — references a v0.7.1 release that has no corresponding section in this file, so reviewers/users can't trace the 0.6.0 → 0.8.0 path. Either backfill 0.7.0/0.7.1 entries or note the gap.
4. (Doc bug, category 3) Link reference footnotes were not updated
At the bottom of the file (lines 78–83):
[Unreleased]: https://github.com/OpenNHP/opennhp/compare/v0.6.0...HEADis now stale; should bev0.8.0...HEAD.- No
[0.8.0]:reference link was added. - The trailing newline at EOF was also stripped (line 83 of the diff:
-[0.3.6]: …\n→+[0.3.6]: …with no newline). POSIX text files should end with a newline.
5. (Quality, category 5) Duplicate changelog bullet
Lines 13–14 both read **server-plugin:** polish basic auth-plugin demo page, only differing by commit hash (one is the squash, one is the merge). Worth deduping in the final release notes.
The 0.8.0 jump from 0.7.1 for a UI polish feature is also notable (semver would suggest a patch), but that's a release-please commit-convention question, not something to fix in this PR.
Recommended fix: add an extra-files mapping for nhp/version/VERSION in release-please config, and either reconfigure release-please to honor the existing Keep-a-Changelog format or convert the file fully to release-please's auto format (and backfill the 0.7.x entries) so future releases stay consistent.
🤖 I have created a release beep boop
0.8.0 (2026-05-07)
Features
This PR was generated with Release Please. See documentation.