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

Follow button on profile is not working json parse error #15916

Closed
flozero opened this issue Jan 1, 2022 · 3 comments · Fixed by #16067
Closed

Follow button on profile is not working json parse error #15916

flozero opened this issue Jan 1, 2022 · 3 comments · Fixed by #16067
Assignees
Labels
bug always open for contribution

Comments

@flozero
Copy link

flozero commented Jan 1, 2022

Describe the bug

When clicking on follow button is not working because of a json parse error

To Reproduce

  1. go to https://dev.to/stack-labs?signin=true
  2. notice that you have in error in the console because of a json parse error
  3. click on follow
  4. its not working

Expected behavior

We should be able to follow

Screenshots

Screen Shot 2022-01-01 at 4 18 36 PM

Desktop (please complete the following information):

  • OS, version: macbook m1 pro max
  • Browser, version: chrome latest

Smartphone (please complete the following information):

  • Device:
  • OS, version:
  • Browser, version:

Additional context

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2022

Thanks for the issue, we will take it into consideration! Our team of engineers is busy working on many types of features, please give us time to get back to you.

Feature requests that require more discussion may be closed. Read more about our feature request process on forem.dev.

To our amazing contributors: issues labeled type: bug are always up for grabs, but for feature requests, please wait until we add a ready for dev before starting to work on it.

To claim an issue to work on, please leave a comment. If you've claimed the issue and need help, please ping @forem/oss. The OSS Community Manager or the engineers on OSS rotation will follow up.

For full info on how to contribute, please check out our contributors guide.

@djuber
Copy link
Contributor

djuber commented Jan 2, 2022

this is reproducible.

Checking the code for initializeAllUserFollowButtons and experimenting a little - it appears a user's data is causing an issue

const buttons = document.querySelectorAll(
  '.follow-action-button.follow-user:not([data-fetched])',
);

Array.from(buttons, (button) => { console.log(button.dataset.info); JSON.parse(button.dataset.info);})
{"id": 383123, "className": "User", "style": "full", "name": "Chabane R."} base.js:7495:61
{"id": 383123, "className": "User", "style": "full", "name": "Chabane R."} base.js:7495:61
{"id": 399496, "className": "User", "style": "full", "name": "Λ\: Olivier Revial"} base.js:7495:61
# parse error occurs here.

Taking the raw data-info from the inspector and pasting it into node - it looks like the doubled backslash character is causing an issue... I assume when we render this we need to double that again (sending a sequence '\\' instead of '\')

The capital lambda character (which has no special meaning) is not a problem, but the double backslash appears to break the handling of the user name (there are a few users with this pattern, each are unparseable and cause all follow buttons to be non-functioning).

> JSON.parse("{\"id\": 399496, \"className\": \"User\", \"style\": \"full\", \"name\": \"Λ\\: Olivier Revial\"}")
Uncaught SyntaxError: Unexpected token : in JSON at position 64
> JSON.parse("{\"id\": 399496, \"className\": \"User\", \"style\": \"full\", \"name\": \"\\: Olivier Revial\"}")
Uncaught SyntaxError: Unexpected token : in JSON at position 63
> JSON.parse("{\"id\": 399496, \"className\": \"User\", \"style\": \"full\", \"name\": \"\\\\: Olivier Revial\"}")
{ id: 399496, className: 'User', style: 'full', name: '\\: Olivier Revial' }
  id: 399496,
  className: 'User',
  style: 'full',
  name: '\\: Olivier Revial'
}

In the database the user's name is just 'Λ\: Olivier Revial' - I think a good summary for this is "users with slashes in the name break the follow button" - it also happens on the user profile page - for example at https://dev.to/clementbosc

The linked issue was the same problem caused by a user with quote characters in their name, and the underlying issue appears to be the way we're embedding usernames in the data-info attribute is not json parse safe.

@djuber
Copy link
Contributor

djuber commented Jan 2, 2022

related #14704

@citizen428 citizen428 added the bug always open for contribution label Jan 3, 2022
@jeremyf jeremyf self-assigned this Jan 6, 2022
jeremyf added a commit that referenced this issue Jan 6, 2022
With these changes we're ensuring that we "stringify" a "user's name.

Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them.

Closes #15916, #14704

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

On this PR's branch (e.g.,
`jeremyf/patching-parse-error-for-user-with-control-characters-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 7, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 7, 2022
While working on #15916 (via #15983 and later #15994) I was exploring if
we needed to stringify JSON values.  I also injected a few `try, catch,
debug` areas.

Consolidating the parsing does not appear to adversely affect things.
jeremyf added a commit that referenced this issue Jan 10, 2022
While working on #15916 (via #15983 and later #15994) I was exploring if
we needed to stringify JSON values.  I also injected a few `try, catch,
debug` areas.

Consolidating the parsing does not appear to adversely affect things.
jeremyf added a commit that referenced this issue Jan 10, 2022
Below are the grep results of searching for ArticleSerializer (note
there are no remaining `Search::ArticleSerializer' references).

```log
app/services/search/reading_list.rb:118:
  `Search::ReadingListArticleSerializer`
app/services/search/article.rb:39:
  `Homepage::ArticleSerializer`
app/services/homepage/fetch_articles.rb:43:
  `Homepage::ArticleSerializer`
app/serializers/search/reading_list_article_serializer.rb:2:
  `class ReadingListArticleSerializer < ApplicationSerializer`
app/serializers/homepage/article_serializer.rb:2:
  `class ArticleSerializer < ApplicationSerializer`
spec/serializers/search/reading_list_article_serializer_spec.rb:3:
  `RSpec.describe Search::ReadingListArticleSerializer do`
```

This relates to exploration around #15916 and the attempted solutions in

Definitely want to keep pruning unused code.
jeremyf added a commit that referenced this issue Jan 10, 2022
Below are the grep results of searching for ArticleSerializer (note
there are no remaining `Search::ArticleSerializer' references).

Using `ripgrep` (e.g., `rg`), I have the following from the `main`
branch.

```log
> rg ArticleSerializer
app/services/search/reading_list.rb:
  Search::ReadingListArticleSerializer
app/services/search/article.rb:
  Homepage::ArticleSerializer
app/services/homepage/fetch_articles.rb:
  Homepage::ArticleSerializer
spec/serializers/search/reading_list_article_serializer_spec.rb:
  RSpec.describe Search::ReadingListArticleSerializer do
app/serializers/search/reading_list_article_serializer.rb:
  class ReadingListArticleSerializer < ApplicationSerializer
app/serializers/homepage/article_serializer.rb:
  class ArticleSerializer < ApplicationSerializer
```

Definitely want to keep pruning unused code.

This relates to exploration around #15916 and the attempted solutions in
jeremyf added a commit that referenced this issue Jan 10, 2022
Below are the grep results of searching for ArticleSerializer (note
there are no remaining `Search::ArticleSerializer' references).

Using `ripgrep` (e.g., `rg`), I have the following from the `main`
branch.

```log
> rg ArticleSerializer
app/services/search/reading_list.rb:
  Search::ReadingListArticleSerializer
app/services/search/article.rb:
  Homepage::ArticleSerializer
app/services/homepage/fetch_articles.rb:
  Homepage::ArticleSerializer
spec/serializers/search/reading_list_article_serializer_spec.rb:
  RSpec.describe Search::ReadingListArticleSerializer do
app/serializers/search/reading_list_article_serializer.rb:
  class ReadingListArticleSerializer < ApplicationSerializer
app/serializers/homepage/article_serializer.rb:
  class ArticleSerializer < ApplicationSerializer
```

Definitely want to keep pruning unused code.

This relates to exploration around #15916 and the attempted solutions in
jeremyf added a commit that referenced this issue Jan 10, 2022
Prior to this commit, we had two places that need to know the nuances
ofquerying for tag flares and what we should include in our queries for
serialization.

With this commit, we're factoring towards a common source of knowledge
and providing a much needed test for the expected output of this
serialization.

Loosely related to #15916, #15983, #15994, and #16032.
jeremyf added a commit that referenced this issue Jan 10, 2022
Prior to this commit, we had two places that need to know the nuances
ofquerying for tag flares and what we should include in our queries for
serialization.

With this commit, we're factoring towards a common source of knowledge
and providing a much needed test for the expected output of this
serialization.

Loosely related to #15916, #15983, #15994, and #16032.
jeremyf added a commit that referenced this issue Jan 11, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 13, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 13, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 13, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 13, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
jeremyf added a commit that referenced this issue Jan 14, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes #15916, #14704

Supersedes #15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
headline-design pushed a commit to headline-design/forem-1 that referenced this issue Mar 11, 2022
While working on forem#15916 (via forem#15983 and later forem#15994) I was exploring if
we needed to stringify JSON values.  I also injected a few `try, catch,
debug` areas.

Consolidating the parsing does not appear to adversely affect things.
headline-design pushed a commit to headline-design/forem-1 that referenced this issue Mar 11, 2022
Below are the grep results of searching for ArticleSerializer (note
there are no remaining `Search::ArticleSerializer' references).

Using `ripgrep` (e.g., `rg`), I have the following from the `main`
branch.

```log
> rg ArticleSerializer
app/services/search/reading_list.rb:
  Search::ReadingListArticleSerializer
app/services/search/article.rb:
  Homepage::ArticleSerializer
app/services/homepage/fetch_articles.rb:
  Homepage::ArticleSerializer
spec/serializers/search/reading_list_article_serializer_spec.rb:
  RSpec.describe Search::ReadingListArticleSerializer do
app/serializers/search/reading_list_article_serializer.rb:
  class ReadingListArticleSerializer < ApplicationSerializer
app/serializers/homepage/article_serializer.rb:
  class ArticleSerializer < ApplicationSerializer
```

Definitely want to keep pruning unused code.

This relates to exploration around forem#15916 and the attempted solutions in
headline-design pushed a commit to headline-design/forem-1 that referenced this issue Mar 11, 2022
Prior to this commit, we had two places that need to know the nuances
ofquerying for tag flares and what we should include in our queries for
serialization.

With this commit, we're factoring towards a common source of knowledge
and providing a much needed test for the expected output of this
serialization.

Loosely related to forem#15916, forem#15983, forem#15994, and forem#16032.
headline-design pushed a commit to headline-design/forem-1 that referenced this issue Mar 11, 2022
Prior to this commit, we were somewhat naively rendering Hash style data
attributes in our ERB templates.  By rendering each hash attribute
separately, we were rendering characters that could break the
javascript (e.g. double hack or backslash `"` or `\`).

By moving to this view_object rendering, we leverage Rails's `to_json`
behavior to ensure properly escaped values.  As part of this exercise, I
generalized the method to allow for other places to benefit from this
behavior.

This generalization also helps ensure that we have a more conformant
rendering (e.g. we should always have an :id, :className, and :name
value in our data-info hash).

_Note: I've updated the user's names for Cypress tests as they are more
likely to catch the particular issue than anything else.  I assume that
I'm going to break some cypress tests and will need some help fixing
them._

Closes forem#15916, forem#14704

Supersedes forem#15983

How to test locally:

Assuming you have seeded database (e.g. `rails db:seed`), checkout the
"main" branch.  Then in `rails console` find a user that's written articles:

```ruby
user = Article.last.user

user.update(name: "\\: #{user.name}")

user.articles.each(&:save)
```

Now, again on the "main" branch, start your application (e.g.,
`bin/startup`).

Then get a logged in and a logged out browser session going.  Open your
web inspector and open console.  Then go to the local instances homepage
(e.g., http://localhost:3000) and look for JS errors.

On the main branch, you should see an exception around
`JSON.parse(button.data.info)` (assuming that the `user`'s article is
rendered on the homepage).

Then go to the user's page (e.g. https://localhost:3000/:user-slug) and
look for JS parse errors.

On this PR's branch (e.g.,
`jeremyf/take-two-at-resolving-gh-15916`)
you shouldn't see those console errors.

More importantly, the Follow buttons should work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug always open for contribution
Projects
None yet
4 participants