Skip to content

fix(balancer): pass cleaned team name to changePasscodeHashForTeam - #1292

Merged
commjoen merged 10 commits into
OWASP:mainfrom
youknowme19:fix/reset-passcode-teamname-mismatch
Aug 17, 2026
Merged

fix(balancer): pass cleaned team name to changePasscodeHashForTeam#1292
commjoen merged 10 commits into
OWASP:mainfrom
youknowme19:fix/reset-passcode-teamname-mismatch

Conversation

@youknowme19

Copy link
Copy Markdown

Thank you for submitting a pull request to the WrongSecrets Party!

What kind of changes does this PR include?

  • Fixes or refactors
  • Platform support
  • A new feature
  • Additional documentation
  • Something else

Description

Fixes a bug in the passcode reset handler where the raw team cookie value (req.teamname containing the t- prefix) is passed to changePasscodeHashForTeam instead of the cleaned team name (req.cleanedTeamname). This caused the balancer to attempt patching a non-existent deployment/namespace named t-t-teamname-wrongsecrets on Kubernetes.

Checklist:

  • All the contributions made are solely the work of me and my co-authors
  • I tested the changes in this PR (if applicable)
  • I added tests to ensure my change works (if applicable)
  • The PR passes pre-commit hooks and automated tests

@commjoen

Copy link
Copy Markdown
Collaborator

Hey @youknowme19 thank you for your PR! Can you maybe add a cypress test to ensure the flow will continue to work from here on?

@youknowme19

Copy link
Copy Markdown
Author

Thanks for the review! Absolutely, I’ll add a Cypress test covering the passcode reset flow end-to-end. I have classes today, so I’ll work on the test later today and push the update to this PR once I’ve verified it properly.

@github-actions

Copy link
Copy Markdown

🚀 Preview Build Complete!

Your pull request has been built successfully. However, since this is from a fork, preview images cannot be pushed to the organization's container registry.

Testing Your Changes

To test your changes, you can build and deploy locally:

# Clone this PR
git fetch origin pull/1292/head:pr-1292
git checkout pr-1292

# Build and deploy locally
./build-and-deploy.sh

# Or for minikube
./build-and-deploy-minikube.sh

# Port forward to access locally
kubectl port-forward service/wrongsecrets-balancer 3000:3000

Alternative: Manual Build

# Build images locally
cd wrongsecrets-balancer
docker build -t my-wrongsecrets-balancer:test .
cd ../cleaner
docker build -t my-cleaner:test .

# Deploy with custom images using Helm
helm repo add wrongsecrets https://owasp.org/wrongsecrets-ctf-party
helm repo update

helm install my-preview wrongsecrets/wrongsecrets-ctf-party \
  --set balancer.repository=my-wrongsecrets-balancer \
  --set balancer.tag=test \
  --set wrongsecretsCleanup.repository=my-cleaner \
  --set wrongsecretsCleanup.tag=test \
  --set balancer.imagePullPolicy=Never \
  --set wrongsecretsCleanup.imagePullPolicy=Never

Why Can't Images Be Pushed?

External contributors don't have write permissions to the organization's GitHub Container Registry. This is a security measure to protect the organization's packages.


This preview was automatically generated for PR #1292

@youknowme19
youknowme19 force-pushed the fix/reset-passcode-teamname-mismatch branch from bb9a872 to f29f7eb Compare August 12, 2026 09:55
@youknowme19

Copy link
Copy Markdown
Author

The requested Cypress regression test has been added to the existing team_workflow.cy.js flow.

The E2E CI run is currently failing before reaching the new passcode-reset section:

  • admin_login.cy.js fails because [data-test-id="passcode-input"] is not found.
  • team_workflow.cy.js fails earlier in the existing team workflow with an Axios 500/unhandled promise rejection, before reaching the new reset-passcode regression test.

The Balancer, Cleaner, Minikube build/container, Pre-commit, and CodeQL checks all pass.

I haven't made any additional changes to work around the E2E failures because they appear to occur before the new regression test is reached. Please let me know if you'd like me to investigate the underlying E2E/environment failure separately.

@commjoen

Copy link
Copy Markdown
Collaborator

I haven't made any additional changes to work around the E2E failures because they appear to occur before the new regression test is reached. Please let me know if you'd like me to investigate the underlying E2E/environment failure separately.

Yes, can you please have a look? updating your branch showed one to succeed, which still feels flakey. It could help if the logs are captured during the test for instance.

@youknowme19

Copy link
Copy Markdown
Author

Thanks!
Sure I'll look into it

@youknowme19

Copy link
Copy Markdown
Author

I investigated the flaky E2E failure.

The 500 appears to come from an existing readiness race in the Balancer rather than from the passcode-reset changes in this PR.

When a newly created team's Kubernetes deployment has not yet been registered, getJuiceShopInstanceForTeamname() can return undefined after the 404 is handled. awaitReadiness() then destructures readyReplicas from that undefined value, resulting in the 500 seen by Cypress.

This also explains the flakiness: if the deployment is registered before the first readiness check, the test proceeds normally; otherwise the first readiness request can fail with 500.

I also noticed that the mock Kubernetes path does not reproduce this because it returns a ready instance directly.

I don't think we should fix this in #1292 since it is unrelated to the passcode-reset change. I suggest keeping this PR focused and opening a separate issue/PR for the readiness race.

For the CI debugging, I can also add failure-only Kubernetes/pod/Balancer log capture so future E2E failures include the relevant server-side context. Would you prefer that as a separate small PR, or should I leave the workflow unchanged and just rerun this job?

@commjoen

Copy link
Copy Markdown
Collaborator

Can you make 2 additional PRs to add the logs and to fix the issue in the flows please? Then we have a more stable and easy to debug cypress setup

@commjoen

Copy link
Copy Markdown
Collaborator

hope to test this PR next working week to check if evertyhing works

@youknowme19

Copy link
Copy Markdown
Author

Hi @commjoen, both follow-up PRs are now completed and merged:

#1296 — added Kubernetes/Cypress diagnostics on E2E failure
#1297 — fixed the Balancer readiness race causing the intermittent 500

Both PRs passed all checks and were merged into main.

#1292 itself remains focused on the passcode-reset fix. I'll leave it as-is for your review/testing next week.

@commjoen

Copy link
Copy Markdown
Collaborator

I still woner what went wrong with #1292, but hope to look at that next week.

@youknowme19

Copy link
Copy Markdown
Author

CI update: The passcode-reset E2E flow targeted by this PR is now passing successfully in team_workflow.cy.js, including the reset request returning 200 OK.

The remaining admin_login.cy.js failure appears to be unrelated to this PR. I compared the relevant test and deployment configuration against origin/main and confirmed that the failure is also present on main, with no corresponding changes in this PR.

I have therefore kept the PR scoped to the original team-name/passcode-reset issue rather than bundling an unrelated CI configuration change.

@commjoen
commjoen merged commit ad2fc3f into OWASP:main Aug 17, 2026
19 checks passed
@youknowme19
youknowme19 deleted the fix/reset-passcode-teamname-mismatch branch August 18, 2026 17:10
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.

2 participants