fix(directory): harden VMACS query construction by rlorenzo · Pull Request #226 · ucdavis/VIPER

rlorenzo · 2026-06-16T21:47:55Z

What

Two small hardening changes to VMACSService.Search:

URL-encode the login ID — Uri.EscapeDataString(loginID ?? string.Empty) before interpolating into the query URL, so reserved characters can't alter the request. Also drops a no-op .ToString().
Extract the AUTH token to a named constant — VmacsAuthToken instead of a magic literal in the URL string.

Why

Both surfaced during the CodeQL/agy review work. loginID is a DB-sourced campus login (AaudUser.LoginId, passed by both DirectoryController call sites), so exploitability is low; the AUTH value is a fixed internal-endpoint token, not a rotating secret. This is hygiene, not a vulnerability fix.

Scope note

The agy review suggested further changes (read the response stream directly instead of string→bytes→MemoryStream, and externalize the token/URL via config + DI). Those are behavior-adjacent / architectural and were deliberately kept out to keep this PR scoped; recommend a separate ticket for the static→DI refactor.

codecov-commenter · 2026-06-16T21:48:39Z

Bundle Report

Bundle size has no change ✅

codecov-commenter · 2026-06-16T21:49:27Z

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.51%. Comparing base (1a0996e) to head (81e0601).

Files with missing lines	Patch %	Lines
web/Areas/Directory/Services/VMACSService.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
- Coverage   44.51%   44.51%   -0.01%     
==========================================
  Files         895      895              
  Lines       51760    51761       +1     
  Branches     4827     4828       +1     
==========================================
  Hits        23041    23041              
- Misses      28149    28150       +1     
  Partials      570      570

Flag	Coverage Δ
backend	`44.67% <0.00%> (-0.01%)`	⬇️
frontend	`41.32% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Copilot

Pull request overview

This PR hardens the Directory area’s VMACS lookup by URL-encoding the loginID value before it’s interpolated into the outbound query string, preventing reserved characters from altering the request.

Changes:

Encode loginID via Uri.EscapeDataString(loginID ?? string.Empty) before building the VMACS query URL.
Remove a no-op .ToString() call on an already-string interpolated value.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

rlorenzo · 2026-06-17T05:10:37Z

    public class VMACSService
    {
+        // Fixed token required by the internal VMACS trust endpoint (not a rotating secret).
+        private const string VmacsAuthToken = "06232005";


@bsedwards Should this be a config value?

- URL-encode the login ID (Uri.EscapeDataString) before interpolating it into the VMACS query URL so reserved characters can't alter the request. - Extract the fixed AUTH token into a named VmacsAuthToken constant instead of a magic literal in the URL string. - Drop a no-op .ToString() on the interpolated string. loginID is a DB-sourced campus login (low exploitability) and the AUTH token is a fixed internal-endpoint value, so this is hygiene/hardening, not a vulnerability fix.

rlorenzo · 2026-06-26T06:13:19Z

@coderabbitai full review

coderabbitai · 2026-06-26T06:13:24Z

✅ Action performed

Full review finished.

coderabbitai · 2026-06-26T06:13:27Z

📝 Walkthrough

Walkthrough

VMACSService now stores the VMACS AUTH token in a constant and uses it when building the trust query. The loginID value is URL-encoded before being added to the request.

Changes

VMACS trust query update

Layer / File(s)	Summary
Named AUTH token and encoded loginID `web/Areas/Directory/Services/VMACSService.cs`	Adds a private `VmacsAuthToken` constant and builds the VMACS trust query with `Uri.EscapeDataString(loginID ?? string.Empty)` for `loginID` and the constant for `AUTH`.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

bsedwards

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change to VMACS query construction.
Description check	✅ Passed	The description directly matches the code changes and scope of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/vmacs-url-encode-loginid

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

rlorenzo · 2026-06-26T15:38:24Z

@coderabbitai review

coderabbitai · 2026-06-26T15:38:58Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@web/Areas/Directory/Services/VMACSService.cs`:
- Around line 27-28: The URL construction in VMACSService is still embedded in
the lookup logic, so pull the request-building portion out into a small helper
method (for example, in the same service near the login lookup path) that takes
the login ID and returns the encoded query string. Then add focused tests
against that helper to cover null, spaces, +, &, and = cases so the
Uri.EscapeDataString behavior and request format in the lookup path are
independently verifiable.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bd9770b9-c6b2-4fcf-885c-070c57819dc7

📥 Commits

Reviewing files that changed from the base of the PR and between 1a0996e and 81e0601.

📒 Files selected for processing (1)

web/Areas/Directory/Services/VMACSService.cs

coderabbitai · 2026-06-26T15:41:19Z

+            string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty);
+            string request = $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={VmacsAuthToken}";


📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Make the query builder independently testable.

Line 27-28 changed lookup semantics, but this path still has no patch coverage. Pull the URL construction into a small helper and add cases for null, spaces, +, &, and = so encoding regressions fail fast.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Areas/Directory/Services/VMACSService.cs` around lines 27 - 28, The URL construction in VMACSService is still embedded in the lookup logic, so pull the request-building portion out into a small helper method (for example, in the same service near the login lookup path) that takes the login ID and returns the encoded query string. Then add focused tests against that helper to cover null, spaces, +, &, and = cases so the Uri.EscapeDataString behavior and request format in the lookup path are independently verifiable.

rlorenzo requested a review from Copilot June 16, 2026 22:36

Copilot started reviewing on behalf of rlorenzo June 16, 2026 22:37 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

rlorenzo force-pushed the fix/vmacs-url-encode-loginid branch from c372293 to 2767fcb Compare June 17, 2026 02:07

rlorenzo changed the title ~~fix(directory): URL-encode VMACS login ID in query~~ fix(directory): harden VMACS query construction Jun 17, 2026

rlorenzo requested a review from Copilot June 17, 2026 02:07

Copilot started reviewing on behalf of rlorenzo June 17, 2026 02:07 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

rlorenzo commented Jun 17, 2026

View reviewed changes

rlorenzo force-pushed the fix/vmacs-url-encode-loginid branch from 2767fcb to 4a2c7c9 Compare June 24, 2026 01:51

rlorenzo force-pushed the fix/vmacs-url-encode-loginid branch from 4a2c7c9 to 81e0601 Compare June 26, 2026 03:05

coderabbitai Bot reviewed Jun 26, 2026

View reviewed changes

		string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty);
		string request = $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={VmacsAuthToken}";

Uh oh!

Conversation

rlorenzo commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Scope note

Uh oh!

codecov-commenter commented Jun 16, 2026

Bundle Report

Uh oh!

codecov-commenter commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rlorenzo Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

rlorenzo commented Jun 26, 2026

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Uh oh!

rlorenzo commented Jun 26, 2026

Uh oh!

coderabbitai Bot commented Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rlorenzo commented Jun 16, 2026 •

edited

Loading

codecov-commenter commented Jun 16, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading

coderabbitai Bot commented Jun 26, 2026 •

edited

Loading