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

Ensure HTML escaping only occurs for strings #148

Merged
merged 1 commit into from Mar 7, 2019

Conversation

alexsanford
Copy link
Contributor

Fixes #124

Note that I was not able to reproduce the fatal mentioned in that issue.

Also note that I built this on top of #146 in order to work with Sensei 2.0. I will rebase when #146 is merged.

Testing instructions

See #124. Ensure content drip works.

@alexsanford alexsanford requested review from jom and donnapep March 6, 2019 23:54
@alexsanford alexsanford self-assigned this Mar 6, 2019
@alexsanford alexsanford added this to the 1.1.0 milestone Mar 6, 2019
Copy link
Contributor

@jom jom left a comment

Choose a reason for hiding this comment

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

This looks like it'll do the job. It feels a bit weird to drop sanitization of non-strings here, but it seems like the only method it is actually used (Scd_Ext_Lesson_Admin::content_drip_lesson_meta_content()) things get re-sanitized.

I tried looking more at version history and couldn't see where we might have stored a value as DateTime. Seems pretty strange.

@alexsanford alexsanford merged commit 06f0c9e into change/deps-check Mar 7, 2019
@alexsanford alexsanford deleted the fix/html-escaping-strings branch March 7, 2019 20:27
@jom jom restored the fix/html-escaping-strings branch March 7, 2019 22:13
@donnapep donnapep deleted the fix/html-escaping-strings branch March 8, 2019 11:57
@jom jom restored the fix/html-escaping-strings branch March 8, 2019 12:02
@jom
Copy link
Contributor

jom commented Mar 8, 2019

@donnapep Restored branch. This PR needs to be re-opened for merging into master.

@alexsanford
Copy link
Contributor Author

Sorry for the blunder on this one! I couldn't reopen this PR, so I opened a new one instead: #152

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

2 participants