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
Extended image checkings #15
Conversation
karma.conf.js
Outdated
@@ -63,6 +63,42 @@ module.exports = (config) => { | |||
pattern: srcOriginalRecursivePath, | |||
included: false, | |||
served: true | |||
}, | |||
{ | |||
pattern: 'test/images/*.png', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern allows us to use a regexp instead of specifying each image type independently.
Please write a RegExp that covers all of types that you want to using with unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to use something like (jpg|gif|png|svg), but it doesn't work. I will try regexp expression.
src/validationHelper.ts
Outdated
export function isImageUrlAllowed(url: string): boolean { | ||
// Excludes all URLs that don't contain .gif .jpg .png or .svg extensions and don't start from "http(s)://". | ||
// As a result -- also excludes all directives such as "javascript:", "data:" and "blob:". | ||
return (/^https?:\/\/.+\.(gif|jpg|png|svg)$/i).test(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering why this RegExp excludes base64 encoded images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need base64 images?
test/data/someplaintext.txt
Outdated
@@ -0,0 +1 @@ | |||
Plain text with some data taht is used in unit tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling mistake: that
test/images/sun.svg
Outdated
@@ -0,0 +1,14 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- Generated by IcoMoon.io --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this SVG free? Can you generate your own svg file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is free, but I will generate my own svg anyway
supportedTypes = ["image/png", "image/jpeg", "image/gif", "image/svg+xml"]; | ||
|
||
if (supportedTypes.indexOf(contentType) > -1) { | ||
return imageCheckResultCallBack(true, contentType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that it'd be nice to return image data as the third argument of callback.
What is your point of view?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this info may be useful, especially if we use HEAD request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't make a sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent!
No description provided.