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

[WIP][FurAffinityBridge] Add new bridge #1083

Merged
merged 10 commits into from Jul 26, 2019

Conversation

Roliga
Copy link
Contributor

@Roliga Roliga commented Mar 28, 2019

New bridge for furaffinity.net.

While github.com/boothale/faexport already exists I wanted something integrated with rss-bridge.

FurAffinity requires an authenticated user for most things, so this bridge uses an authentication cookie from a shared FurAffinity account.
As far as I can tell these cookes don't really normally expire, and any write operations with the account should require a password. FurAffinity does have an access log for the account which shows IP addressess of requests which is not ideal though. I'm not sure if there is some better way to handle this. Maybe requiring the user to specify their own cookies, but that doesn't feel too great either. Suggestions are welcome.

This depends on #1057 to work.

@teromene
Copy link
Member

teromene commented Apr 4, 2019

Hmmm.... I kinda have mixed feelings about including the cookie in the code...
On one hand, we did previously agreed on the fact that bridge shouldn't include "private" information, such as cookies...
On the other hand, if the cookie never expires and if there is no way to do anything with the cookie only, it might be considered as the internal API keys that we use sometimes...
I will need input from the others before merging...

@Roliga
Copy link
Contributor Author

Roliga commented Apr 4, 2019

Yeah the whole shared account thing doesn't seem like the best. Also about it being read-only: I imagine one could use it to "log out" without a password which would invalidate the cookie which isn't great either.

@logmanoriginal
Copy link
Member

On the other hand, if the cookie never expires and if there is no way to do anything with the cookie only, it might be considered as the internal API keys that we use sometimes

I agree. Cookies reveal no more information than any other API key, so we should treat it the same.

I imagine one could use it to "log out" without a password which would invalidate the cookie which isn't great either.

Even if this happens, users will eventually figure out to insert their own personal cookie to make it work.


If bridges had access to the settings (separate from the RSS-Bridge settings), it would be possible for server Administrators to specify their own keys without changing the source code. That way we don't have to share keys publicly.

It might be worth looking into this if more bridges require authentication in the future.

Copy link
Member

@logmanoriginal logmanoriginal left a comment

Choose a reason for hiding this comment

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

Impressive bridge, looks good to me 👍

Find below a few comments for consideration. I have no objection against the cookie as mentioned above.

->find('.alt1 table div.no_overflow', 0)
->innertext;

$this->items[] = $item;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of if($limit-- === 0) break; above, you can do

Suggested change
$this->items[] = $item;
$this->items[] = $item;
if (count($this->items) >= $limit) break;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason this was done this way was to allow limit = -1 to mean "unlimited", like suggested in the relevant bridge parameters' titles. I added a comment about this in 2563019.

Maybe doing it the way you suggested and having limit = 9999 or such is more obvious for users though. What do you think?

$cache = ($this->getInput('cache') === true);

foreach($html->find('section.gallery figure') as $figure) {
if($limit-- === 0) break;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

bridges/FurAffinityBridge.php Outdated Show resolved Hide resolved
'Content-Type: application/x-www-form-urlencoded',
'Cookie: ' . self::FA_AUTH_COOKIE
);
return getSimpleHTMLDOM($this->getURI(), $header, $opts);
Copy link
Member

Choose a reason for hiding this comment

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

Can you check what happens if the cookie is invalid?
It might return error 401 which could be translated to a proper error message for the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly it returns a 200 whether the cookie is valid or not, with an error message on some pages. That could be parsed but for most lists of posts the results are just filtered and otherwise look normal with no error message, so I'm not sure how a faulty cookie could be detected properly.

I suppose sending an additional request at the beginning of collectData to something like the users settings page would reliably return a page that could be parsed for error messages so maybe that's an option. Do you think it'd be worth it?

bridges/FurAffinityBridge.php Show resolved Hide resolved
@logmanoriginal
Copy link
Member

Please don't forget to remove the '[WIP]' if this is ready for merge.

@teromene
Copy link
Member

teromene commented May 8, 2019

I am agreeing with @logmanoriginal about the credentials. Let's test it while we haven't got any better solution

@teromene teromene merged commit cf525c9 into RSS-Bridge:master Jul 26, 2019
@teromene
Copy link
Member

Thanks for the PR and for taking in account our suggestions ! Merged !

infominer33 pushed a commit to web-work-tools/rss-bridge that referenced this pull request Apr 17, 2020
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.

None yet

3 participants