-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add Lingvanex Integration #12
Conversation
controller/controller.go
Outdated
@@ -43,6 +43,10 @@ func ServeStaticFile(w http.ResponseWriter, r *http.Request) { | |||
fs := http.FileServer(filesDir) | |||
|
|||
w.Header().Set("Access-Control-Allow-Origin", "*") | |||
w.Header().Set("Content-Security-Policy", "trusted-types") |
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 need to explicitly disable trusted types? We should be able to just leave it off and use the default ("no trusted types").
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.
would it break anything if we set this to require-trusted-types-for 'script'
? google has this in csp report-only mode which indicates they're trying to turn it on
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.
^ to be clear, that's just on el_main.js and translateelement.css, not element.js. are we using/proxying these resources?
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.
@diracdeltas yes we proxy all requests including static files
if err != nil { | ||
log.Errorf("Error closing response body stream: %v", err) | ||
} | ||
}() | ||
|
||
// Set Header | ||
w.Header().Set("Content-Type", msResp.Header["Content-Type"][0]) | ||
w.Header().Set("Content-Type", lnxResp.Header["Content-Type"][0]) |
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.
Could it be something else than 'application/json'?
if err != nil { | ||
log.Errorf("Error closing response body stream: %v", err) | ||
} | ||
}() | ||
|
||
// Set Header | ||
w.Header().Set("Content-Type", msResp.Header["Content-Type"][0]) | ||
w.Header().Set("Content-Type", lnxResp.Header["Content-Type"][0]) | ||
w.Header().Set("Access-Control-Allow-Origin", "*") // same as Google response |
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.
@diracdeltas @fmarier
Do we need extra headers here (translate_a/t
) ?
Adding x-content-type-options: nosniff
looks reasonable..
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.
best to just copy whatever headers Google uses for their responses. but probably the headers from https://github.com/brave/go-translate/pull/12/files#diff-3c5bb5f48211873c58fcba055dcae2ac7b1958969219e06e1508d76d485dace7R47-R50 should apply here.
// Also deal with duplicate mappings for ar, en, es, fr, pt | ||
supportedLangList := []byte(`[{"code_alpha_1":"auto","codeName":"Auto","rtl":false},{"code_alpha_1":"en","codeName":"English","rtl":false},{"code_alpha_1":"fr","codeName":"French","rtl":false},{"code_alpha_1":"pl","codeName":"Polish","rtl":false},{"code_alpha_1":"zh-Hans","codeName":"Chinese (Simplified)","rtl":false},{"code_alpha_1":"vi","codeName":"Vietnamese","rtl":false},{"code_alpha_1":"pt","codeName":"Portuguese","rtl":false},{"code_alpha_1":"nl","codeName":"Dutch","rtl":false},{"code_alpha_1":"de","codeName":"German","rtl":false},{"code_alpha_1":"ja","codeName":"Japanese","rtl":false},{"code_alpha_1":"es","codeName":"Spanish","rtl":false},{"code_alpha_1":"ro","codeName":"Romanian","rtl":false},{"code_alpha_1":"tr","codeName":"Turkish","rtl":false},{"code_alpha_1":"it","codeName":"Italian","rtl":false},{"code_alpha_1":"hi","codeName":"Hindi","rtl":false},{"code_alpha_1":"ru","codeName":"Russian","rtl":false}]`) | ||
langs := make([]Language, 0) | ||
err := json.Unmarshal(supportedLangList, &langs) |
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 believe we shouldn't construct such structs on each requests.
I would rather make them const or static.
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.
this is old logic - also it seems Lingvanex have adopted ISO alpha-2 format now so we can do away with the conversion altogether
reqBody.From = lnx_from | ||
reqBody.To = lnx_to | ||
reqBody.TranslateMode = "html" | ||
reqBody.Data = qVals |
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.
This code processes the user input as it and we can't add more validation here.
@linhkikuchi
Do we have any isolation for lnx container in case of RCE/another security issue?
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.
Also maybe we need do some sanity check here like len(qVals) < n, qVals[i] < k
to avoid intentional out of memory attack?
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.
all containers can communicate with each other within internal network, so we don't have any issues
|
||
for _, v := range langs { | ||
if v.IsoCode == gLangCode { | ||
return v.IsoCode, nil |
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 seems we always return original gLangCode
or an error.
So why we store a list of structure supportedLangList
? If we store only the codes in a map it will simplify things.
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.
also old logic, will be refactored
As discussed, we will refactor and clean-up after merge with #14 |
Resolves #10