Skip to content

[General] Fix: No longer replace token/sid with redacted #1442

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

Merged
merged 2 commits into from
Jun 12, 2022

Conversation

CommandMC
Copy link
Collaborator

It seems like arrays are passed by reference in JS, which means the getRunnerCallWithoutCredentials function was altering the actual commandParts passed to Legendary/GOGDL. By copying the array before passing it, this no longer happens


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

This sends a *copy* of commandParts to getRunnerCallWithoutCredentials,
which should avoid it overwriting the sid/token
passed to legendary/gogdl
@CommandMC CommandMC requested review from imLinguin and Nocccer and removed request for imLinguin June 6, 2022 14:03
@CommandMC CommandMC changed the base branch from main to beta June 6, 2022 14:05
@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jun 6, 2022
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe add function documentation via tsdoc so people know that they should provide a copy of commandParts.

@Nocccer Nocccer changed the title No longer replace token/sid with redacted [General] Fix: No longer replace token/sid with redacted Jun 8, 2022
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CommandMC CommandMC added pr:ready-to-merge This PR is fully ready for merge. and removed pr:ready-for-review Feature-complete, ready for the grind! :P labels Jun 10, 2022
@flavioislima flavioislima merged commit 220e1ac into beta Jun 12, 2022
@flavioislima flavioislima deleted the fix/no-redacted branch June 12, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants