Jc/fix code smells #84
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
| @@ -41,6 +41,7 @@ class ConsolePopout extends Component<Props> { | |||
|
|
|||
| this.externalWindow!.document.title = 'VM Popout'; | |||
|
|
|||
| // Sonarcloud thinks this line is a security issue | |||
| this.externalWindow!.addEventListener('beforeunload', (ev: BeforeUnloadEvent) => { | |||
There was a problem hiding this comment.
@jasekiw @Cliftonz Sonarcloud thinks this is a security issue because we aren't verifying the origin of the message that we're listening to. But this is a reference to a window that we opened ourselves, so I don't think there's any real security risk here? I could be wrong, just wanted to check before I marked the issue resolved on Sonarcloud.
There was a problem hiding this comment.
Someone could intercept our call back to a users and put their own vnc messages in there.
I remember talking about this with Jason but I do not remember what we decided on.
There was a problem hiding this comment.
Normally the solution is to just verify the event origin, which I tried, but the BeforeUnloadEvent type that we're using doesn't seem to have a origin property. So I'm not sure what the proper solution here would be, at least not without some significant refactoring.
Let's see if Jason has any ideas, he built this component.
There was a problem hiding this comment.
@Cliftonz How could someone intercept our callback? They would have to do cross site scripting which would be an even worse exploit.
There was a problem hiding this comment.
@Cliftonz We're talking about an event handler that asks the user if they are sure if they want to exit, how much damage could exploiting this method really do?
There was a problem hiding this comment.
@jasekiw I'm with you here, I think. I can mark this as won't fix if you're okay with that.
| @@ -16,7 +16,7 @@ export interface UploadByUrlForm extends NamedUpload { | |||
| const NameSchema: ObjectSchemaDefinition<NamedUpload> = {name: string().required('Required').min(3, 'Must have at least 3 characters')}; | |||
| const UploadFormSchema = object<UploadForm>({ | |||
| ...NameSchema, | |||
| file: object().nullable() as ObjectSchema<File> | |||
| file: object() as ObjectSchema<File> | |||
There was a problem hiding this comment.
This is a dangerous suggestion, it breaks the validation message but it was also broken before in a different way.
it should be this:
| file: object() as ObjectSchema<File> | |
| file: object<File>().nullable().required('File is required') |
There was a problem hiding this comment.
Committed this change.
| @@ -41,6 +41,7 @@ class ConsolePopout extends Component<Props> { | |||
|
|
|||
| this.externalWindow!.document.title = 'VM Popout'; | |||
|
|
|||
| // Sonarcloud thinks this line is a security issue | |||
| this.externalWindow!.addEventListener('beforeunload', (ev: BeforeUnloadEvent) => { | |||
There was a problem hiding this comment.
@Cliftonz How could someone intercept our callback? They would have to do cross site scripting which would be an even worse exploit.
| @@ -41,6 +41,7 @@ class ConsolePopout extends Component<Props> { | |||
|
|
|||
| this.externalWindow!.document.title = 'VM Popout'; | |||
|
|
|||
| // Sonarcloud thinks this line is a security issue | |||
| this.externalWindow!.addEventListener('beforeunload', (ev: BeforeUnloadEvent) => { | |||
There was a problem hiding this comment.
@Cliftonz We're talking about an event handler that asks the user if they are sure if they want to exit, how much damage could exploiting this method really do?
| @@ -3,7 +3,6 @@ import {BrowserTypes} from '../types/actionTypes'; | |||
| import {WindowState} from '../types/BrowserState'; | |||
| import Action from '../types/redux'; | |||
|
|
|||
| // Code smell on this function signature | |||
| function windowSize(state: WindowState = {height: NaN, width: NaN}, action: Action) { | |||
There was a problem hiding this comment.
This is how redux is designed to be used, this code smell needs to be disabled
There was a problem hiding this comment.
from what I see it is just saying to put the defaults at the end.
https://sonarcloud.io/project/issues?id=ius-csg_cslabs-webapp&issues=AXsiTV_Kw3kkXW2HHOS-&open=AXsiTV_Kw3kkXW2HHOS-
There was a problem hiding this comment.
Jason is right, if you try to flip it, it breaks redux. I marked this as a false positive.
| @@ -9,12 +9,10 @@ import {persistGlobalStore, persistRootReducer} from './persistance'; | |||
|
|
|||
| const configureStore = (initialState?: DeepPartial<WebState>, onReady?: () => void) => { | |||
| const root = persistRootReducer(combineReducers(rootReducer(history))); | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
This used to break without ts-ignore, it doesn't any more?
There was a problem hiding this comment.
Well, it was working when I opened this PR. But now I've merged dev in and the linter is mad, so I'm not sure what changed. Either way I'll just put it back.
| const composeEnhancers: typeof compose = (typeof window !== 'undefined' && window['__REDUX_DEVTOOLS_EXTENSION_COMPOSE__']) || compose; | ||
| const storeInstance = createStore( | ||
| root, | ||
| initialState, | ||
| // Code smell on this |
There was a problem hiding this comment.
This is completely false, it does change the type expressions and is necessary
There was a problem hiding this comment.
Yeah, that's why I removed this comment. I was using the comments to help me keep track of what code smells I hadn't fixed yet. Once I realized this was a false positive I removed the comment and marked the smell resolved.
|
Kudos, SonarCloud Quality Gate passed! |
Fixes the rest of the code smells in the front-end.