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

Handle errors from reddit #35

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Conversation

Tokarak
Copy link
Contributor

@Tokarak Tokarak commented Jan 31, 2024

A 401 code is still an Ok(<...>) response, so the error handling was in the wrong place.

Hopefully the conditional logic (json["reason"] == "Unauthorized") is correct;

The information reported to the browser should be improved.

It's obvious that Libreddit wasn't designed with error-handling in mind.

Linked issue: #22

A 401 code is still an Ok(<...>) response
@Tokarak
Copy link
Contributor Author

Tokarak commented Jan 31, 2024

Draft until the efficacy and the correctness of the conditional logic (see above) is verified.

@sigaloid
Copy link
Member

It's obvious that Libreddit wasn't designed with error-handling in mind.

😅 That's because we try to avoid logging at all costs to preserve privacy. Probably not ideal for cases like this. That's why I want to eventually add more debug-only logging so if there's a problem, someone can run it in debug mode for themselves

@Tokarak Tokarak marked this pull request as ready for review February 1, 2024 15:26
@Tokarak
Copy link
Contributor Author

Tokarak commented Feb 1, 2024

image The error page is now a little more informative. It should probably be formatted better. I'm most worried about the privacy/security concerns for the host (the issue was there before): the status code and reason could be abducted, and an attacker could glean information from this error page.

@Tokarak Tokarak force-pushed the pr-fix-error-handling branch from d851b90 to be4e9ec Compare February 1, 2024 15:40
@sigaloid
Copy link
Member

sigaloid commented Feb 1, 2024

I'm not sure how much of a vulnerability it is to have a user of the site know the status code returned from reddit - what kind of information would that leak?

@Tokarak
Copy link
Contributor Author

Tokarak commented Feb 2, 2024

Probably not a concern, but it can leak information that Reddit knows about the host which is unknown by the client. I was thinking information about the Reddit-facing network interface, but I doubt Reddit would leak anything about that.

A rationale is that the host is just a proxy for the client, and so everything Reddit sends should be forwarded to the client anyway.

@Tokarak
Copy link
Contributor Author

Tokarak commented Feb 2, 2024

Just confirmed working!

@sigaloid
Copy link
Member

sigaloid commented Feb 2, 2024

Sounds great, thanks! Merging.

@sigaloid sigaloid merged commit 469d099 into redlib-org:main Feb 2, 2024
3 checks passed
nohoster pushed a commit to nohoster/redlib that referenced this pull request Feb 24, 2024
* Fix error handling logic

A 401 code is still an Ok(<...>) response

* Fix json key

* Run `cargo fmt`
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