From 1e9e51bfd2d27f236ffb083d18d3358649a6eda1 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 16 Jun 2026 14:45:56 -0700 Subject: [PATCH 1/4] fix(directory): harden VMACS query construction - 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. --- web/Areas/Directory/Services/VMACSService.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/web/Areas/Directory/Services/VMACSService.cs b/web/Areas/Directory/Services/VMACSService.cs index bc71f751a..581031923 100644 --- a/web/Areas/Directory/Services/VMACSService.cs +++ b/web/Areas/Directory/Services/VMACSService.cs @@ -7,6 +7,9 @@ namespace Viper.Areas.Directory.Services { public class VMACSService { + // Fixed token required by the internal VMACS trust endpoint (not a rotating secret). + private const string VmacsAuthToken = "06232005"; + private static HttpClient sharedClient = new() { BaseAddress = new Uri("https://vmacs-vmth.vetmed.ucdavis.edu"), @@ -21,7 +24,8 @@ protected VMACSService() { } public static async Task Search(String? loginID) { - string request = $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={loginID}&format=CHRIS4&AUTH=06232005".ToString(); + string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty); + string request = $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={VmacsAuthToken}"; using HttpResponseMessage response = await sharedClient.GetAsync(request); if (!response.IsSuccessStatusCode) { From b778bc5e02fb7f54c6ae6890e567fa4d16d45ada Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 30 Jun 2026 13:25:24 -0700 Subject: [PATCH 2/4] fix(directory): load VMACS auth token from config, not source Read the VMACS trust-endpoint AUTH token from the Credentials config section instead of a hardcoded constant, matching how DB and other secrets are sourced. Requires the Credentials:VmacsAuthToken value to exist in AWS SSM Parameter Store before deploy. --- web/Areas/Directory/Services/VMACSService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/Areas/Directory/Services/VMACSService.cs b/web/Areas/Directory/Services/VMACSService.cs index 581031923..bc5e6ad3d 100644 --- a/web/Areas/Directory/Services/VMACSService.cs +++ b/web/Areas/Directory/Services/VMACSService.cs @@ -7,8 +7,8 @@ namespace Viper.Areas.Directory.Services { public class VMACSService { - // Fixed token required by the internal VMACS trust endpoint (not a rotating secret). - private const string VmacsAuthToken = "06232005"; + private static string VmacsAuthToken => + HttpHelper.GetSetting("Credentials", "VmacsAuthToken") ?? string.Empty; private static HttpClient sharedClient = new() { From 19f294194ae22f1ae0a2034e6374f8434d0bb04c Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 30 Jun 2026 13:56:55 -0700 Subject: [PATCH 3/4] refactor(directory): extract testable VMACS query builder Pull request-path construction out of Search into BuildSearchPath so the URL-encoding is unit-testable; add cases for null, spaces, +, &, and = to guard against encoding regressions. Also marks the shared HttpClient readonly. --- test/Areas/Directory/VMACSServiceTest.cs | 32 ++++++++++++++++++++ web/Areas/Directory/Services/VMACSService.cs | 12 +++++--- 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 test/Areas/Directory/VMACSServiceTest.cs diff --git a/test/Areas/Directory/VMACSServiceTest.cs b/test/Areas/Directory/VMACSServiceTest.cs new file mode 100644 index 000000000..37a5c7512 --- /dev/null +++ b/test/Areas/Directory/VMACSServiceTest.cs @@ -0,0 +1,32 @@ +using Viper.Areas.Directory.Services; + +namespace Viper.test.Areas.Directory +{ + public class VMACSServiceTest + { + [Theory] + [InlineData(null, "")] + [InlineData("", "")] + [InlineData("jdoe", "jdoe")] + [InlineData("john doe", "john%20doe")] + [InlineData("a+b", "a%2Bb")] + [InlineData("a&b", "a%26b")] + [InlineData("a=b", "a%3Db")] + public void BuildSearchPath_UrlEncodesLoginId(string? loginID, string expectedFind) + { + string path = VMACSService.BuildSearchPath(loginID, "TESTAUTH"); + + Assert.Equal( + $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={expectedFind}&format=CHRIS4&AUTH=TESTAUTH", + path); + } + + [Fact] + public void BuildSearchPath_IncludesAuthToken() + { + string path = VMACSService.BuildSearchPath("jdoe", "SECRET123"); + + Assert.Contains("&AUTH=SECRET123", path); + } + } +} diff --git a/web/Areas/Directory/Services/VMACSService.cs b/web/Areas/Directory/Services/VMACSService.cs index bc5e6ad3d..f2995d300 100644 --- a/web/Areas/Directory/Services/VMACSService.cs +++ b/web/Areas/Directory/Services/VMACSService.cs @@ -10,7 +10,7 @@ public class VMACSService private static string VmacsAuthToken => HttpHelper.GetSetting("Credentials", "VmacsAuthToken") ?? string.Empty; - private static HttpClient sharedClient = new() + private static readonly HttpClient sharedClient = new() { BaseAddress = new Uri("https://vmacs-vmth.vetmed.ucdavis.edu"), }; @@ -23,9 +23,7 @@ protected VMACSService() { } /// public static async Task Search(String? loginID) { - - string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty); - string request = $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={VmacsAuthToken}"; + string request = BuildSearchPath(loginID, VmacsAuthToken); using HttpResponseMessage response = await sharedClient.GetAsync(request); if (!response.IsSuccessStatusCode) { @@ -54,5 +52,11 @@ protected VMACSService() { } } return null; } + + internal static string BuildSearchPath(string? loginID, string authToken) + { + string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty); + return $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={authToken}"; + } } } From fa5b4a6b58aa523d4e46981d5d38c8c25f429fd3 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 30 Jun 2026 19:04:58 -0700 Subject: [PATCH 4/4] ci: rework Development merge check for the direct-merge flow Rework the advisory Development merge check so it fits how the team actually merges into Development: a direct `git merge` + push (npm run merge), not a PR. The old merged-PR detection never matched, so the check was meaningless. It is now a single red/green PR check: green when the head commit is in Development (detected by ancestry, with a merged-PR squash fallback), red when it is not. It re-evaluates on every PR commit, and a push-on-Development run re-runs any still-red PR so it flips to green once the branch lands there. Advisory only; it never blocks merging. --- .github/workflows/development-merge-check.yml | 127 ++++++++++++------ 1 file changed, 89 insertions(+), 38 deletions(-) diff --git a/.github/workflows/development-merge-check.yml b/.github/workflows/development-merge-check.yml index 1d7c6aeac..7a66c9f69 100644 --- a/.github/workflows/development-merge-check.yml +++ b/.github/workflows/development-merge-check.yml @@ -1,25 +1,39 @@ name: Development Merge Check -# Advisory, NON-BLOCKING check for the team workflow that branches should merge -# into Development before main. When a PR into main is opened whose branch is not -# yet in Development, this surfaces a warning annotation. It never fails the check, -# so it does not block merging — it is purely informational. +# Advisory, NON-BLOCKING check for the team convention that branches merge into +# Development before main. # -# "Merged to Development" is detected by looking for a MERGED pull request whose -# base is Development and whose head is this branch. This repo squash-merges, so -# commit ancestry against Development would give false negatives after a squash. +# It is a single check on the PR - this job's own pass/fail: +# green when the PR head commit is already in Development +# red when it is not (a reminder, not a hard failure; it does not block a +# merge unless someone marks it a required check) +# +# "In Development" is detected by ancestry, because the team merges into +# Development with a direct `git merge` + push (`npm run merge`), not a PR, so the +# PR head becomes an ancestor of Development. A merged-PR lookup is kept as a +# squash-merge fallback. +# +# Triggers: +# pull_request - evaluate this PR's head on every commit (this is the verdict). +# push: Development - a branch can land in Development without a new PR commit, +# so re-run each still-red PR's check to flip it green. +# workflow_dispatch - manual refresh. on: pull_request: - types: [opened, reopened, synchronize, edited, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review] branches: [main] + push: + branches: [Development] + workflow_dispatch: permissions: contents: read pull-requests: read + actions: write concurrency: - group: dev-merge-check-${{ github.event.pull_request.number }} + group: dev-merge-check-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true jobs: @@ -27,42 +41,79 @@ jobs: name: Development merge check (advisory) runs-on: ubuntu-latest steps: - - name: Warn if not merged to Development first + - name: Check Development merge status uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | const DEV_BRANCH = 'Development'; - - const pr = context.payload.pull_request; + const WORKFLOW = 'development-merge-check.yml'; const { owner, repo } = context.repo; - const headRef = pr.head.ref; - // Fork-safe: fall back to the base repo owner if the head repo is gone. - const headOwner = pr.head.repo ? pr.head.repo.owner.login : owner; - // Has this branch already been merged into Development? - // Robust to squash merges: find a merged PR with base=Development, head=this branch. - const devPulls = await github.paginate(github.rest.pulls.list, { - owner, - repo, - state: 'closed', - base: DEV_BRANCH, - head: `${headOwner}:${headRef}`, - per_page: 100, - }); - const mergedToDev = devPulls.find((p) => p.merged_at); + // Detail string if the head is in Development, else null. + async function mergedDetail(headRef, headSha, headOwner) { + // Direct merge: npm run merge makes a real merge commit, so the head + // is contained in Development ("behind" or "identical"). + try { + const cmp = await github.rest.repos.compareCommitsWithBasehead({ + owner, repo, basehead: `${DEV_BRANCH}...${headSha}`, + }); + if (cmp.data.status === 'behind' || cmp.data.status === 'identical') { + return `Branch "${headRef}" is on ${DEV_BRANCH}: its commits (through ${headSha.slice(0, 7)}) are already in ${DEV_BRANCH}.`; + } + } catch (e) { + core.info(`compare ${DEV_BRANCH}...${headSha} failed: ${e.message}`); + } + // Fallback: a PR/squash merge into Development. + const devPulls = await github.paginate(github.rest.pulls.list, { + owner, repo, state: 'closed', base: DEV_BRANCH, + head: `${headOwner}:${headRef}`, per_page: 100, + }); + const mergedPr = devPulls.find((p) => p.merged_at); + return mergedPr + ? `Branch "${headRef}" is on ${DEV_BRANCH} (merged via PR #${mergedPr.number}).` + : null; + } - if (mergedToDev) { - core.info( - `Code reached ${DEV_BRANCH} via #${mergedToDev.number} ` + - `(merged ${mergedToDev.merged_at}).` - ); + // pull_request: this job's pass/fail IS the advisory check for the PR. + if (context.eventName === 'pull_request') { + const pr = context.payload.pull_request; + const headOwner = pr.head.repo ? pr.head.repo.owner.login : owner; + const detail = await mergedDetail(pr.head.ref, pr.head.sha, headOwner); + if (detail) { + core.info(detail); + } else { + core.setFailed( + `Branch "${pr.head.ref}" is not on ${DEV_BRANCH} yet. Team workflow ` + + `routes branches through ${DEV_BRANCH} before main. Advisory only; ` + + `it does not block merging.`, + ); + } return; } - // Not in Development yet: warn, but do not fail (advisory only). - core.warning( - `Branch "${headRef}" has not been merged into ${DEV_BRANCH} yet. Team workflow ` + - `routes branches through ${DEV_BRANCH} before main. This is advisory and does ` + - `not block merging.`, - { title: 'Not yet merged to Development' } - ); + // push to Development / manual dispatch: a branch may have just landed + // in Development without a new PR commit. Re-run each still-red PR's + // check so its verdict refreshes against the updated Development. + const prs = await github.paginate(github.rest.pulls.list, { + owner, repo, state: 'open', base: 'main', per_page: 100, + }); + const runs = await github.paginate(github.rest.actions.listWorkflowRuns, { + owner, repo, workflow_id: WORKFLOW, event: 'pull_request', per_page: 100, + }); + const latestBySha = new Map(); + for (const r of runs) { + if (!latestBySha.has(r.head_sha)) latestBySha.set(r.head_sha, r); + } + for (const pr of prs) { + const run = latestBySha.get(pr.head.sha); + if (run?.conclusion === 'failure') { + try { + await github.rest.actions.reRunWorkflowFailedJobs({ + owner, repo, run_id: run.id, + }); + core.info(`PR #${pr.number}: re-running advisory check (was red).`); + } catch (e) { + core.warning(`PR #${pr.number}: re-run failed: ${e.message}`); + } + } + }