fix(balancer): pass cleaned team name to changePasscodeHashForTeam - #1292
Conversation
|
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? |
|
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. |
🚀 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 ChangesTo 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:3000Alternative: 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=NeverWhy 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 |
bb9a872 to
f29f7eb
Compare
|
The requested Cypress regression test has been added to the existing The E2E CI run is currently failing before reaching the new passcode-reset section:
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. |
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. |
|
Thanks! |
|
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, 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? |
|
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 |
|
hope to test this PR next working week to check if evertyhing works |
|
Hi @commjoen, both follow-up PRs are now completed and merged: #1296 — added Kubernetes/Cypress diagnostics on E2E failure 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. |
|
I still woner what went wrong with #1292, but hope to look at that next week. |
|
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. |
Thank you for submitting a pull request to the WrongSecrets Party!
What kind of changes does this PR include?
Description
Fixes a bug in the passcode reset handler where the raw team cookie value (
req.teamnamecontaining thet-prefix) is passed tochangePasscodeHashForTeaminstead of the cleaned team name (req.cleanedTeamname). This caused the balancer to attempt patching a non-existent deployment/namespace namedt-t-teamname-wrongsecretson Kubernetes.Checklist: