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

cli auth add: do not fail with extraneous characters #3521

Conversation

hopeseekr
Copy link
Contributor

@hopeseekr hopeseekr commented Dec 27, 2020

Description

The Solution:
I have hardened auth add by stripping out everything from the third '/' to the end of the instance URL.

The Problem:
When adding an authorization for the peertube-cli, before this commit you could not have anything after the domain_name:port.

For instance, if there was a trailing / in your instance URL, before this commit it will always fail with

expected 200 "OK", got 404 "Not Found".

It took me over 20 minutes to figure out that this was the problem.

Related issues

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 👍 yes, light tests as follows are enough
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help

Please describe the tests that you ran to verify your changes.

I attempted to run the entire test suite, but after many failed starts and one really long one, I cancelled the run after it had been going 37+ minutes. I then attempted to run the CLI tests alone, but that, too, I cancelled after 27 minutes. I then commented out everything inside server/tests/cli/index.ts except the peertube.ts. I then attempted to run it via the command found in the CONTRIBUTING.md:

TS_NODE_FILES=true npm run mocha -- --exit -r ts-node/register -r tsconfig-paths/register --bail server/tests/cli/index.ts 

but all I ended up with was:

 TypeError: Cannot read property 'app' of undefined

So yeah, I was unable to run the tests, but I did create tests for my own feature.

This is the first time I've ever attempted to run Mocha tests before, and I found it very confusing. I am used to the downright simplicity of phpunit tests/MyTest.php or even phpunit --filter beginningOfMyTestCaseName. I stumbled around worse than a drunken pirate fresh off a won parlay.

@hopeseekr hopeseekr force-pushed the 3520-cli_auth_add_trailing_slash branch 2 times, most recently from 9e5ae77 to 13ff40a Compare December 27, 2020 16:31
@rigelk rigelk linked an issue Dec 27, 2020 that may be closed by this pull request
@rigelk rigelk changed the title (#3520) [cli] Hardened auth add: No longer fails with extraneous characters. cli auth add: do not fail with extraneous characters Dec 27, 2020
@rigelk
Copy link
Collaborator

rigelk commented Dec 27, 2020

@hopeseekr could you remove all semicolons to please the linter?

…raneous characters.

**The Solution:**
I have hardened `auth add` by stripping out everything from the third '/' to the end of the instance URL.

**The Problem:**
When adding an authorization for the peertube-cli, before this commit you could not have anything after the domain_name:port.

For instance, if there was a trailing / in your instance URL, before this commit it will always fail with

    expected 200 "OK", got 404 "Not Found".

It took me over 20 minutes to figure out that this was the problem.

See Issue Chocobozzz#3091.
@hopeseekr hopeseekr force-pushed the 3520-cli_auth_add_trailing_slash branch from 13ff40a to 8e76aa1 Compare December 27, 2020 20:30
@hopeseekr hopeseekr changed the title cli auth add: do not fail with extraneous characters [cli] Hardened auth add: No longer fails when given full URLs. Dec 27, 2020
@rigelk rigelk changed the title [cli] Hardened auth add: No longer fails when given full URLs. cli auth add: do not fail with extraneous characters Dec 27, 2020
@Chocobozzz Chocobozzz merged commit 8e76aa1 into Chocobozzz:develop Dec 30, 2020
@Chocobozzz
Copy link
Owner

Thanks @hopeseekr

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.

Accept trailing slash with peertube auth add
3 participants