Add security improvements to the codebase#2
Conversation
Add security improvements to the codebase. * **linkpreview.go**: Add input validation for `link` parameter in `Parse` and `ParseFromReader` functions. Add security headers to HTTP request in `Parse` function. * **parser.go**: Add error handling for potential security vulnerabilities in `Parse` function. Add validation for `MetaTags` and `LinkTags` in `readAllTags` function. * **parser_og.go**: Add validation for `tagContent` in `parseOGMeta`, `parseOGImages`, and `parseOGVideos` functions. * **parser_twitter.go**: Add validation for `tagContent` in `parseTwitterMeta` function. * **parser_favicons.go**: Add validation for `tag.Href` in `parseMaybeFavicon` function. * **README.md**: Add section on security guidelines and best practices. * **go.mod**: Add security-related dependencies. * **go.sum**: Update to include new dependencies. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/hungvminh/go-linkpreview?shareId=XXXX-XXXX-XXXX-XXXX).
Improve security in linkpreview package
|
| request.Header.Set("X-Content-Type-Options", "nosniff") | ||
| request.Header.Set("X-Frame-Options", "DENY") | ||
| request.Header.Set("X-XSS-Protection", "1; mode=block") |
There was a problem hiding this comment.
I think this is incorrect usage of secure headers. They are for response headers only.
Please see: https://cheatsheetseries.owasp.org/cheatsheets/HTTP_Headers_Cheat_Sheet.html
| } | ||
|
|
||
| func Parse(link string, options ...ConfigOption) (ParserResult, error) { | ||
| if err := validateURL(link); err != nil { |
There was a problem hiding this comment.
Maybe this is not necessary as http.NewRequest() and http.DefaultClient.Do() should handle the invalid URL correctly.
| wh := strings.Split(tag.Sizes, "x") | ||
| if len(wh) == 2 { | ||
| image.Width = parseInt(wh[0]) | ||
| image.Height = parseInt(wh[1]) | ||
| } |
There was a problem hiding this comment.
Please restore indentation of this block.
| } | ||
| } | ||
|
|
||
| func validateMetaTag(tag *MetaTag) error { |
There was a problem hiding this comment.
HTML content may be not correct 100%. The use of this function can cause error to be returned due to a single incorrect piece of data. I suggest adding a config item to ParserConfig, says SkipInvalidTags, with default value false (to be back-ward compatible). When it is turned on, every invalid tags will be ignored instead of returning error to client.
|
|
||
| func validateMetaTag(tag *MetaTag) error { | ||
| if tag.Name == "" && tag.Property == "" { | ||
| return errors.New("meta tag must have either a name or property attribute") |
There was a problem hiding this comment.
It is recommended not to use dynamic error. You should place this error in top of the file like:
var (
errTagInvalid = errors.New("tag is invalid")
)
// In here
return fmt.Errorf("%w: meta tag must have either a name or property attribute", errTagInvalid)Another option is to make this function just return bool as we only need to know whether we should keep the tag or not.
| return nil | ||
| } | ||
|
|
||
| func validateLinkTag(tag *LinkTag) error { |
There was a problem hiding this comment.
The same comment for this function.
| "github.com/PuerkitoBio/goquery" | ||
| ) | ||
|
|
||
| func validateURL(link string) error { |
2 participants
Add security improvements to the codebase.
linkparameter inParseandParseFromReaderfunctions. Add security headers to HTTP request inParsefunction.Parsefunction. Add validation forMetaTagsandLinkTagsinreadAllTagsfunction.tagContentinparseOGMeta,parseOGImages, andparseOGVideosfunctions.tagContentinparseTwitterMetafunction.tag.HrefinparseMaybeFaviconfunction.