[1/2]: Add campaign news functionality #1446
Conversation
Test: builds
We currently use innerHTML to show article's content, which is prone to XSS attacks. Since the support for adding campaign related news is expanded to the campaign's organizator its better to sanitize the article content before its shown to user
For now it will be used only as a way for author or admin to read draft before publishing it.
sashko9807
changed the title
|
✅ Tests will run for this PR. Once they succeed it can be merged. |
Needed to accomodate latest db changes
DOMpurify needs window object to work which is not available when rendering the component in server, thus we are making sure CampaignNewsList is rendered only on client
We now send everything through the authorization header
sashko9807
force-pushed
the
add-campaign-news-functionality
branch
from
e593048 to
ff9822b
Compare
The first and last name of the logged in user will be used as default values if the field is empty
…image Apply this to EditForm As well
sashko9807
force-pushed
the
add-campaign-news-functionality
branch
from
94e93bb to
79f373a
Compare
sashko9807
force-pushed
the
add-campaign-news-functionality
branch
from
79f373a to
5d1bd74
Compare
sashko9807
force-pushed
the
add-campaign-news-functionality
branch
from
5d1bd74 to
afebc6a
Compare
github-actions
bot
removed
the
run tests
| {articles?.map((article, index) => { | ||
| const documents = GetArticleDocuments(article.newsFiles) | ||
| const images = GetArticleGalleryPhotos(article.newsFiles) | ||
| const sanitizedDescription = sanitizeHTML(article.description) |
There was a problem hiding this comment.
well done with introducing secure coding practices to sanitize the HTML!
| <Typography | ||
| component={'div'} | ||
| className={classes.articleDescription} | ||
| dangerouslySetInnerHTML={{ | ||
| __html: sanitizedDescription.slice(0, CHARACTER_LIMIT) + '...', |
There was a problem hiding this comment.
I assume you needed to set the html from Quill via dangerouslySetInnerHTML so that you can limit the length of the description.
A safer would be to do with the Quill editor in readonly mode but I don't know how to limit the text size, so unitl we find how, this way is ok.
There was a problem hiding this comment.
The quill editor seems to use innerHTML under the hood to display the HTML contents, which is the exact same thing as setting dangerouslySetInnerHTML in react. Not quite sure if it is any safer, in regards of outputting HTML, but the HTML content is being sanitized anyway.
Using dangerouslySetInnerHTML also should be tad a bit faster, as the HTML content is displayed directly, rather than having to wait for the React Quill editor to load.
| paddingTop: theme.spacing(0), | ||
| paddingLeft: theme.spacing(0), | ||
| maxWidth: theme.spacing(18), | ||
| marginTop: theme.spacing(-0.5), |
There was a problem hiding this comment.
AFAIK negative margins should be avoided
There was a problem hiding this comment.
Without this negative margin there is a slight offset between the beginning of the content and the beginning of the timeline component(the dot). Adding any sort of margin to the Timeline component breaks it, thus the need to adjust the content, through negative margin.
I could remove the negative margin if that offset doesn't bother anyone, but so far it doesn't seem to cause any conflict with other DOM elements, in either desktop or mobile viewports.
github-actions
bot
removed
the
run tests
…ntend into add-campaign-news-functionality
github-actions
bot
removed
the
run tests
sashko9807
force-pushed
the
add-campaign-news-functionality
branch
from
3913ecb to
6eef16a
Compare
github-actions
bot
removed
the
run tests
Closes #1445 #1352
Motivation and context
This is a desired feature.
Screenshots:
Testing
Steps to test
Affected urls
ADDED -
campaigns/newsADDED -
campaigns/:slug/newsADDED -
campaigns/:slug/news/admin-panelEnvironment
New environment variables:
New or updated dependencies:
dompurifylatestlatest