Skip to content

Request Sanitizing Middleware - #354

Merged
kyle1morel merged 3 commits into
masterfrom
bugfix/remove-null-characters
Aug 26, 2025
Merged

Request Sanitizing Middleware#354
kyle1morel merged 3 commits into
masterfrom
bugfix/remove-null-characters

Conversation

@qhanson55

@qhanson55 qhanson55 commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

Description

Implemented middleware that will sanitize string inputs to remove any unwanted null characters in case users copy and past them into inputs.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Also added some utility function testings and change to ts docs

@qhanson55
qhanson55 requested a review from a team as a code owner August 21, 2025 23:52
@github-actions

Copy link
Copy Markdown

Coverage Report (Frontend)

Totals Coverage
Statements: 41.33% ( 4030 / 9750 )
Methods: 29.51% ( 496 / 1681 )
Lines: 50.13% ( 2526 / 5039 )
Branches: 33.27% ( 1008 / 3030 )

@github-actions

github-actions Bot commented Aug 21, 2025

Copy link
Copy Markdown

Coverage Report (Application)

Totals Coverage
Statements: 47.82% ( 2470 / 5165 )
Methods: 35.69% ( 303 / 849 )
Lines: 61.56% ( 1717 / 2789 )
Branches: 29.47% ( 450 / 1527 )

@qhanson55
qhanson55 force-pushed the bugfix/remove-null-characters branch from ed95b99 to 4743856 Compare August 22, 2025 00:00
Comment thread app/src/middleware/requestSanitizer.ts Outdated
Comment thread app/src/middleware/requestSanitizer.ts Outdated
Comment thread app/tests/unit/middleware/requestSanitizer.spec.ts Outdated
@jujaga
jujaga requested a review from Copilot August 22, 2025 00:25

This comment was marked as outdated.

@qhanson55
qhanson55 force-pushed the bugfix/remove-null-characters branch 2 times, most recently from f7a3c7e to 5217279 Compare August 25, 2025 17:56
Comment thread app/src/services/coms.ts Dismissed
Server-side request forger - https://github.com/bcgov/nr-permitconnect-navigator-service/security/code-scanning/34

Still showing as a one even after uuid check, leaving check but codeql/copilot is missing context here and it isn't the risk it thinks it is as it's a 3rd party api and they're handling the risk.
@qhanson55
qhanson55 force-pushed the bugfix/remove-null-characters branch from 5217279 to 21785b5 Compare August 25, 2025 21:20
@qhanson55
qhanson55 requested a review from Copilot August 26, 2025 17:57

Copilot AI left a comment

Copy link
Copy Markdown

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 middleware to sanitize incoming request bodies by removing null characters from string values. The PR addresses a security concern where users might copy and paste null characters into form inputs, which could cause issues in downstream processing.

  • Adds request sanitization middleware that recursively strips null characters from string values in request bodies
  • Includes comprehensive test coverage for the new sanitization functionality
  • Updates related documentation and removes unused service methods

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
app/src/middleware/requestSanitizer.ts Implements the main middleware function to sanitize request bodies
app/src/middleware/utils.ts Contains utility functions for null character removal and recursive sanitization
app/app.ts Integrates the sanitization middleware into the Express application
app/tests/unit/middleware/requestSanitizer.test.ts Comprehensive tests for the request sanitizer middleware
app/tests/unit/middleware/utils.test.ts Unit tests for the sanitization utility functions
app/tests/unit/utils/utils.test.ts New comprehensive test suite for utility functions
app/src/utils/utils.ts Updates JSDoc comments and adds UUID validation function
app/src/services/coms.ts Removes getObjects method and adds validation to getObject
app/src/controllers/roadmap.ts Updates to use individual object calls instead of bulk getObjects

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread app/src/utils/utils.ts Outdated
Comment thread app/tests/unit/utils/utils.test.ts Outdated
@qhanson55
qhanson55 force-pushed the bugfix/remove-null-characters branch from 21785b5 to 65ee5bb Compare August 26, 2025 18:04
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed for 'nr-permitconnect-navigator-service frontend'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed for 'nr-permitconnect-navigator-service app'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
93.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@kyle1morel
kyle1morel merged commit 46c7655 into master Aug 26, 2025
21 checks passed
@qhanson55
qhanson55 deleted the bugfix/remove-null-characters branch September 8, 2025 22:11
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.

6 participants