Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update some authentication related checks #111

Open
wants to merge 27 commits into
base: dev
Choose a base branch
from
Open

Conversation

jonbarrow
Copy link
Member

Resolves #XXX

Changes:

These changes have been hot-patched in the prod servers for a long time now. This PR simply brings them upstream. Changes include:

  • Only allowing Basic auth in some endpoints
  • Validating that an access token came from a console by checking it's system type
  • Validating the device certificate padding section

Marking as draft for now since I hate the way the "check the system type" feature was implemented. It's a hack, and I want to change it. There's also a couple open issues now directly relating to tokens which we may want to implement here first before merging? Unsure, would like some opinions.

@jonbarrow jonbarrow marked this pull request as ready for review October 19, 2024 14:00
@binaryoverload
Copy link
Member

This reflects the hot patches on the account server. A couple things:

  • In src/services/nnas/routes/support.ts there is sendConfirmationEmail and sendForgotPasswordEmail commented out - do we want to keep these disabled?
  • The following code for src/services/nasc/routes/ac.ts was changed, do we need to carry this over into this PR also? See below:
diff --git a/src/services/nasc/routes/ac.ts b/src/services/nasc/routes/ac.ts
index 1a8c616..d126ed9 100644
--- a/src/services/nasc/routes/ac.ts
+++ b/src/services/nasc/routes/ac.ts
@@ -40,7 +40,7 @@ router.post('/', async (request: express.Request, response: express.Response): P
 
        if (server.maintenance_mode) {
                // TODO - FIND THE REAL UNDER MAINTENANCE ERROR CODE. 110 IS NOT IT
-               response.status(200).send(nascError('110').toString());
+               response.status(503).send(nascError('101').toString());
                return;
        }
 
@@ -112,4 +112,4 @@ async function processServiceTokenRequest(server: HydratedServerDocument, pid: n
        });
 }
 
-export default router;
\ No newline at end of file
+export default router;

binaryoverload and others added 2 commits February 2, 2025 10:26
Fix for Invalid Service Token error - Refresh Token Duration same as Access Token
@jonbarrow
Copy link
Member Author

jonbarrow commented Feb 3, 2025

I'm going to be requesting the review of @DaniElectra and @shutterbug2000 specifically on this one, to make sure I didn't leave any holes in these changes. Specifically the latest commit, as it changes how we validate console bans and such

This is all I had planned for this PR, once reviewed we can merge. I know the PR message mentioned things like bans and token generation, but that can wait for another PR to keep this one's scope down

I have tested this on the Wii U and it seems fine. Have not tested it on the 3DS however

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.

3 participants