Skip to content

Security hardening: XSS, open redirect, CSRF, XXE, memory safety#2

Merged
guimard merged 4 commits intomasterfrom
more-security
Jan 2, 2026
Merged

Security hardening: XSS, open redirect, CSRF, XXE, memory safety#2
guimard merged 4 commits intomasterfrom
more-security

Conversation

@guimard
Copy link
Member

@guimard guimard commented Jan 2, 2026

  • Add HTML escaping for XSS prevention in POST binding forms
  • Add redirect URL validation to prevent open redirects
  • Add CSRF state with nonce and timestamp validation
  • Add session regeneration after authentication
  • Add path traversal protection in file reading
  • Add acceptSso() call for complete SSO validation
  • Add SecureString class for secure memory erasure of private keys
  • Add XXE protection via libxml2 configuration
  • Add input size limits (10MB) for metadata
  • Fix memory management in Identity and Session NewInstance
  • Add security test suite (15 tests)

- Add HTML escaping for XSS prevention in POST binding forms
- Add redirect URL validation to prevent open redirects
- Add CSRF state with nonce and timestamp validation
- Add session regeneration after authentication
- Add path traversal protection in file reading
- Add acceptSso() call for complete SSO validation
- Add SecureString class for secure memory erasure of private keys
- Add XXE protection via libxml2 configuration
- Add input size limits (10MB) for metadata
- Fix memory management in Identity and Session NewInstance
- Add security test suite (15 tests)
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements multiple security hardening measures including XSS prevention through HTML escaping, open redirect protection, CSRF validation with state management, session fixation prevention, path traversal protection, XXE mitigation, and memory safety improvements through a SecureString class. While the security intentions are sound, the implementation contains several critical bugs that need to be addressed before merge.

Key Changes

  • Added HTML escaping for POST binding forms to prevent XSS attacks
  • Implemented redirect URL validation and CSRF state validation with timestamp checking
  • Added SecureString class for secure erasure of private keys and passwords
  • Configured libxml2 to prevent XXE attacks
  • Added input size limits (10MB) for metadata to prevent DoS

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
test/security.test.ts New security test suite with 15 tests, but tests don't validate actual implementations
src/session.cc Fixed memory management with null checks before assignment, but silent error handling
src/identity.cc Fixed memory management with null checks before assignment, but silent error handling
src/secure_string.h New SecureString class for secure memory erasure of sensitive data
src/server.cc Added SecureString usage and metadata size limits, but contains parameter order bug
src/lasso.cc Added XXE protection via libxml2 configuration
lib/express.ts Added XSS escaping, redirect validation, CSRF state, session regeneration, and path traversal protection, but contains critical form action URL bugs
Comments suppressed due to low confidence (1)

src/server.cc:121

  • Incorrect parameter order in function call. The lasso_server_new_from_buffers call passes password as the third argument (line 120) and certificate as the fourth (line 121), but based on the argument parsing logic, the certificate is the third parameter (info[2], lines 100-108) and password is the optional fourth parameter (info[3], lines 110-113). This mismatch could cause the certificate to be interpreted as a password or vice versa. Check the lasso library API documentation to determine the correct parameter order.
  LassoServer* server = lasso_server_new_from_buffers(
    metadata.c_str(),
    privateKey.c_str(),
    password.empty() ? nullptr : password.c_str(),
    certificate.c_str()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@guimard guimard merged commit d58cdc4 into master Jan 2, 2026
6 checks passed
@guimard guimard deleted the more-security branch January 2, 2026 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant